[PATCH v3 0/3] bus: imx-weim

[PATCH v3 0/3] bus: imx-weim

[Sven Van Asbroeck]

Support multiple address ranges per child node on imx-weim.
While we're at it, insert some code which guards against common config
conflicts.

v3:
	added devicetree binding docs

v2:
	corrected acme@... in commit message example

Sven Van Asbroeck (3):
  bus: imx-weim: support multiple address ranges per child node
  dt-bindings: bus: imx-weim: document multiple address ranges per child
    node
  bus: imx-weim: guard against timing configuration conflicts

 .../devicetree/bindings/bus/imx-weim.txt      | 32 ++++++++-
 drivers/bus/imx-weim.c                        | 69 +++++++++++++++----
 2 files changed, 84 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] bus: imx-weim: support multiple address ranges per child node

[Sven Van Asbroeck]

Ensure that timing values for the child node are applied to
all chip selects in the child's address ranges.

Note that this does not support multiple timing settings per
child; this can be added in the future if required.

Example:
&weim {
	acme@0 {
		compatible = "acme,whatever";
		reg = <0 0 0x100>, <0 0x400000 0x800>,
				<1 0x400000 0x800>;
		fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
				0x00000000 0xa0000240 0x00000000>;
	};
};

Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com>
---
 drivers/bus/imx-weim.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index d84996a4528e..5452d22d1bd8 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -46,6 +46,7 @@ static const struct imx_weim_devtype imx51_weim_devtype = {
 };
 
 #define MAX_CS_REGS_COUNT	6
+#define OF_REG_SIZE		3
 
 static const struct of_device_id weim_id_table[] = {
 	/* i.MX1/21 */
@@ -115,27 +116,40 @@ static int __init weim_timing_setup(struct device_node *np, void __iomem *base,
 				    const struct imx_weim_devtype *devtype)
 {
 	u32 cs_idx, value[MAX_CS_REGS_COUNT];
-	int i, ret;
+	int i, ret, reg_idx, num_regs;
 
 	if (WARN_ON(devtype->cs_regs_count > MAX_CS_REGS_COUNT))
 		return -EINVAL;
 
-	/* get the CS index from this child node's "reg" property. */
-	ret = of_property_read_u32(np, "reg", &cs_idx);
+	ret = of_property_read_u32_array(np, "fsl,weim-cs-timing",
+					 value, devtype->cs_regs_count);
 	if (ret)
 		return ret;
 
-	if (cs_idx >= devtype->cs_count)
+	/*
+	 * the child node's "reg" property may contain multiple address ranges,
+	 * extract the chip select for each.
+	 */
+	num_regs = of_property_count_elems_of_size(np, "reg", OF_REG_SIZE);
+	if (num_regs < 0)
+		return num_regs;
+	if (!num_regs)
 		return -EINVAL;
+	for (reg_idx = 0; reg_idx < num_regs; reg_idx++) {
+		/* get the CS index from this child node's "reg" property. */
+		ret = of_property_read_u32_index(np, "reg",
+					reg_idx*OF_REG_SIZE, &cs_idx);
+		if (ret)
+			break;
 
-	ret = of_property_read_u32_array(np, "fsl,weim-cs-timing",
-					 value, devtype->cs_regs_count);
-	if (ret)
-		return ret;
+		if (cs_idx >= devtype->cs_count)
+			return -EINVAL;
 
-	/* set the timing for WEIM */
-	for (i = 0; i < devtype->cs_regs_count; i++)
-		writel(value[i], base + cs_idx * devtype->cs_stride + i * 4);
+		/* set the timing for WEIM */
+		for (i = 0; i < devtype->cs_regs_count; i++)
+			writel(value[i],
+				base + cs_idx * devtype->cs_stride + i * 4);
+	}
 
 	return 0;
 }
-- 
2.17.1

[PATCH v3 2/3] dt-bindings: bus: imx-weim: document multiple address ranges per child node

[Sven Van Asbroeck]

The imx-weim driver was patched to allow correct WEIM configuration
when multiple address ranges are used in a child node.
Update the dt-bindings to reflect this.

Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com>
---
 .../devicetree/bindings/bus/imx-weim.txt      | 32 +++++++++++++++++--
 1 file changed, 29 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/bus/imx-weim.txt b/Documentation/devicetree/bindings/bus/imx-weim.txt
index 683eaf3aed79..dda7d6d66479 100644
--- a/Documentation/devicetree/bindings/bus/imx-weim.txt
+++ b/Documentation/devicetree/bindings/bus/imx-weim.txt
@@ -47,9 +47,9 @@ Optional properties:
 Timing property for child nodes. It is mandatory, not optional.
 
  - fsl,weim-cs-timing:	The timing array, contains timing values for the
-			child node. We can get the CS index from the child
-			node's "reg" property. The number of registers depends
-			on the selected chip.
+			child node. We get the CS indexes from the address
+			ranges in the child node's "reg" property.
+			The number of registers depends on the selected chip:
 			For i.MX1, i.MX21 ("fsl,imx1-weim") there are two
 			registers: CSxU, CSxL.
 			For i.MX25, i.MX27, i.MX31 and i.MX35 ("fsl,imx27-weim")
@@ -80,3 +80,29 @@ Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:
 					0x0000c000 0x1404a38e 0x00000000>;
 		};
 	};
+
+Example for an imx6q-based board, a multi-chipselect device connected to WEIM:
+
+In this case, both chip select 0 and 1 will be configured with the same timing
+array values.
+
+	weim: weim@21b8000 {
+		compatible = "fsl,imx6q-weim";
+		reg = <0x021b8000 0x4000>;
+		clocks = <&clks 196>;
+		#address-cells = <2>;
+		#size-cells = <1>;
+		ranges = <0 0 0x08000000 0x02000000
+			  1 0 0x0a000000 0x02000000
+			  2 0 0x0c000000 0x02000000
+			  3 0 0x0e000000 0x02000000>;
+		fsl,weim-cs-gpr = <&gpr>;
+
+		acme@0 {
+			compatible = "acme,whatever";
+			reg = <0 0 0x100>, <0 0x400000 0x800>,
+				<1 0x400000 0x800>;
+			fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
+				0x00000000 0xa0000240 0x00000000>;
+		};
+	};
-- 
2.17.1

[PATCH v3 3/3] bus: imx-weim: guard against timing configuration conflicts

[Sven Van Asbroeck]

When specifying weim child devices, there is a risk that more than
one timing setting is specified for the same chip select.

The driver cannot support such a configuration.

In case of conflict, this patch will print a warning to the log,
and will ignore the child node in question.

In this example, node acme@1 will be ignored, as it tries to modify
timing settings for CS0:

&weim {
	acme@0 {
		compatible = "acme,whatever";
		reg = <0 0 0x100>;
		fsl,weim-cs-timing = <something>;
	};
	acme@1 {
		compatible = "acme,whatnot";
		reg = <0 0x500 0x100>;
		fsl,weim-cs-timing = <something else>;
	};
};

However in this example, the driver will be happy:

&weim {
        acme@0 {
                compatible = "acme,whatever";
                reg = <0 0 0x100>;
                fsl,weim-cs-timing = <something>;
        };
        acme@1 {
                compatible = "acme,whatnot";
                reg = <0 0x500 0x100>;
                fsl,weim-cs-timing = <something>;
        };
};

Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com>
---
 drivers/bus/imx-weim.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 5452d22d1bd8..dfe74b8c512a 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -46,8 +46,18 @@ static const struct imx_weim_devtype imx51_weim_devtype = {
 };
 
 #define MAX_CS_REGS_COUNT	6
+#define MAX_CS_REGS		6
 #define OF_REG_SIZE		3
 
+struct cs_timing {
+	bool is_applied;
+	u32 regs[MAX_CS_REGS_COUNT];
+};
+
+struct cs_timing_state {
+	struct cs_timing cs[MAX_CS_REGS];
+};
+
 static const struct of_device_id weim_id_table[] = {
 	/* i.MX1/21 */
 	{ .compatible = "fsl,imx1-weim", .data = &imx1_weim_devtype, },
@@ -112,11 +122,14 @@ static int __init imx_weim_gpr_setup(struct platform_device *pdev)
 }
 
 /* Parse and set the timing for this device. */
-static int __init weim_timing_setup(struct device_node *np, void __iomem *base,
-				    const struct imx_weim_devtype *devtype)
+static int __init weim_timing_setup(struct device *dev,
+				    struct device_node *np, void __iomem *base,
+				    const struct imx_weim_devtype *devtype,
+				    struct cs_timing_state *ts)
 {
 	u32 cs_idx, value[MAX_CS_REGS_COUNT];
 	int i, ret, reg_idx, num_regs;
+	struct cs_timing *cst;
 
 	if (WARN_ON(devtype->cs_regs_count > MAX_CS_REGS_COUNT))
 		return -EINVAL;
@@ -145,10 +158,23 @@ static int __init weim_timing_setup(struct device_node *np, void __iomem *base,
 		if (cs_idx >= devtype->cs_count)
 			return -EINVAL;
 
+		/* prevent re-configuring a CS that's already been configured */
+		cst = &ts->cs[cs_idx];
+		if (cst->is_applied && memcmp(value, cst->regs,
+					devtype->cs_regs_count*sizeof(u32))) {
+			dev_err(dev, "fsl,weim-cs-timing conflict on %pOF", np);
+			return -EINVAL;
+		}
+
 		/* set the timing for WEIM */
 		for (i = 0; i < devtype->cs_regs_count; i++)
 			writel(value[i],
 				base + cs_idx * devtype->cs_stride + i * 4);
+		if (!cst->is_applied) {
+			cst->is_applied = true;
+			memcpy(cst->regs, value,
+				devtype->cs_regs_count*sizeof(u32));
+		}
 	}
 
 	return 0;
@@ -162,6 +188,7 @@ static int __init weim_parse_dt(struct platform_device *pdev,
 	const struct imx_weim_devtype *devtype = of_id->data;
 	struct device_node *child;
 	int ret, have_child = 0;
+	struct cs_timing_state ts = {};
 
 	if (devtype == &imx50_weim_devtype) {
 		ret = imx_weim_gpr_setup(pdev);
@@ -170,7 +197,7 @@ static int __init weim_parse_dt(struct platform_device *pdev,
 	}
 
 	for_each_available_child_of_node(pdev->dev.of_node, child) {
-		ret = weim_timing_setup(child, base, devtype);
+		ret = weim_timing_setup(&pdev->dev, child, base, devtype, &ts);
 		if (ret)
 			dev_warn(&pdev->dev, "%pOF set timing failed.\n",
 				child);
-- 
2.17.1

Re: [PATCH v3 2/3] dt-bindings: bus: imx-weim: document multiple address ranges per child node

[Rob Herring]

On Thu,  6 Dec 2018 14:26:32 -0500, Sven Van Asbroeck wrote:
> The imx-weim driver was patched to allow correct WEIM configuration
> when multiple address ranges are used in a child node.
> Update the dt-bindings to reflect this.
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com>
> ---
>  .../devicetree/bindings/bus/imx-weim.txt      | 32 +++++++++++++++++--
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 1/3] bus: imx-weim: support multiple address ranges per child node

[Shawn Guo]

On Thu, Dec 06, 2018 at 02:26:31PM -0500, Sven Van Asbroeck wrote:
> Ensure that timing values for the child node are applied to
> all chip selects in the child's address ranges.
> 
> Note that this does not support multiple timing settings per
> child; this can be added in the future if required.
> 
> Example:
> &weim {
> 	acme@0 {
> 		compatible = "acme,whatever";
> 		reg = <0 0 0x100>, <0 0x400000 0x800>,
> 				<1 0x400000 0x800>;
> 		fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
> 				0x00000000 0xa0000240 0x00000000>;
> 	};
> };
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com>
> ---
>  drivers/bus/imx-weim.c | 36 +++++++++++++++++++++++++-----------
>  1 file changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> index d84996a4528e..5452d22d1bd8 100644
> --- a/drivers/bus/imx-weim.c
> +++ b/drivers/bus/imx-weim.c
> @@ -46,6 +46,7 @@ static const struct imx_weim_devtype imx51_weim_devtype = {
>  };
>  
>  #define MAX_CS_REGS_COUNT	6
> +#define OF_REG_SIZE		3
>  
>  static const struct of_device_id weim_id_table[] = {
>  	/* i.MX1/21 */
> @@ -115,27 +116,40 @@ static int __init weim_timing_setup(struct device_node *np, void __iomem *base,
>  				    const struct imx_weim_devtype *devtype)
>  {
>  	u32 cs_idx, value[MAX_CS_REGS_COUNT];
> -	int i, ret;
> +	int i, ret, reg_idx, num_regs;

As the new variables are not strictly related to the existing ones, they
can be on a new line for cleaner diff log.  And to keep the reverse-tree
order, it will look like the following.

	int reg_idx, num_regs;
	int i, ret;

>  
>  	if (WARN_ON(devtype->cs_regs_count > MAX_CS_REGS_COUNT))
>  		return -EINVAL;
>  
> -	/* get the CS index from this child node's "reg" property. */
> -	ret = of_property_read_u32(np, "reg", &cs_idx);
> +	ret = of_property_read_u32_array(np, "fsl,weim-cs-timing",
> +					 value, devtype->cs_regs_count);
>  	if (ret)
>  		return ret;
>  
> -	if (cs_idx >= devtype->cs_count)
> +	/*
> +	 * the child node's "reg" property may contain multiple address ranges,
> +	 * extract the chip select for each.
> +	 */
> +	num_regs = of_property_count_elems_of_size(np, "reg", OF_REG_SIZE);
> +	if (num_regs < 0)
> +		return num_regs;
> +	if (!num_regs)
>  		return -EINVAL;
> +	for (reg_idx = 0; reg_idx < num_regs; reg_idx++) {
> +		/* get the CS index from this child node's "reg" property. */
> +		ret = of_property_read_u32_index(np, "reg",
> +					reg_idx*OF_REG_SIZE, &cs_idx);

There should be space before and after *.

Shawn

> +		if (ret)
> +			break;
>  
> -	ret = of_property_read_u32_array(np, "fsl,weim-cs-timing",
> -					 value, devtype->cs_regs_count);
> -	if (ret)
> -		return ret;
> +		if (cs_idx >= devtype->cs_count)
> +			return -EINVAL;
>  
> -	/* set the timing for WEIM */
> -	for (i = 0; i < devtype->cs_regs_count; i++)
> -		writel(value[i], base + cs_idx * devtype->cs_stride + i * 4);
> +		/* set the timing for WEIM */
> +		for (i = 0; i < devtype->cs_regs_count; i++)
> +			writel(value[i],
> +				base + cs_idx * devtype->cs_stride + i * 4);
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.17.1
> 

Re: [PATCH v3 2/3] dt-bindings: bus: imx-weim: document multiple address ranges per child node

[Shawn Guo]

On Thu, Dec 06, 2018 at 02:26:32PM -0500, Sven Van Asbroeck wrote:
> The imx-weim driver was patched to allow correct WEIM configuration
> when multiple address ranges are used in a child node.
> Update the dt-bindings to reflect this.
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com>

The bindings patch should go first in the series.

Shawn

Re: [PATCH v3 3/3] bus: imx-weim: guard against timing configuration conflicts

[Shawn Guo]

On Thu, Dec 06, 2018 at 02:26:33PM -0500, Sven Van Asbroeck wrote:
> When specifying weim child devices, there is a risk that more than
> one timing setting is specified for the same chip select.
> 
> The driver cannot support such a configuration.
> 
> In case of conflict, this patch will print a warning to the log,
> and will ignore the child node in question.
> 
> In this example, node acme@1 will be ignored, as it tries to modify
> timing settings for CS0:
> 
> &weim {
> 	acme@0 {
> 		compatible = "acme,whatever";
> 		reg = <0 0 0x100>;
> 		fsl,weim-cs-timing = <something>;
> 	};
> 	acme@1 {
> 		compatible = "acme,whatnot";
> 		reg = <0 0x500 0x100>;
> 		fsl,weim-cs-timing = <something else>;
> 	};
> };
> 
> However in this example, the driver will be happy:
> 
> &weim {
>         acme@0 {
>                 compatible = "acme,whatever";
>                 reg = <0 0 0x100>;
>                 fsl,weim-cs-timing = <something>;
>         };
>         acme@1 {
>                 compatible = "acme,whatnot";
>                 reg = <0 0x500 0x100>;
>                 fsl,weim-cs-timing = <something>;
>         };
> };
> 
> Signed-off-by: Sven Van Asbroeck <TheSven73@googlemail.com>
> ---
>  drivers/bus/imx-weim.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> index 5452d22d1bd8..dfe74b8c512a 100644
> --- a/drivers/bus/imx-weim.c
> +++ b/drivers/bus/imx-weim.c
> @@ -46,8 +46,18 @@ static const struct imx_weim_devtype imx51_weim_devtype = {
>  };
>  
>  #define MAX_CS_REGS_COUNT	6
> +#define MAX_CS_REGS		6

The macro name can easily confuse people with existing
MAX_CS_REGS_COUNT.  By its meaning, I guess MAX_CS_COUNT should be more
accurate?

>  #define OF_REG_SIZE		3
>  
> +struct cs_timing {
> +	bool is_applied;
> +	u32 regs[MAX_CS_REGS_COUNT];
> +};
> +
> +struct cs_timing_state {
> +	struct cs_timing cs[MAX_CS_REGS];
> +};
> +
>  static const struct of_device_id weim_id_table[] = {
>  	/* i.MX1/21 */
>  	{ .compatible = "fsl,imx1-weim", .data = &imx1_weim_devtype, },
> @@ -112,11 +122,14 @@ static int __init imx_weim_gpr_setup(struct platform_device *pdev)
>  }
>  
>  /* Parse and set the timing for this device. */
> -static int __init weim_timing_setup(struct device_node *np, void __iomem *base,
> -				    const struct imx_weim_devtype *devtype)
> +static int __init weim_timing_setup(struct device *dev,
> +				    struct device_node *np, void __iomem *base,
> +				    const struct imx_weim_devtype *devtype,
> +				    struct cs_timing_state *ts)
>  {
>  	u32 cs_idx, value[MAX_CS_REGS_COUNT];
>  	int i, ret, reg_idx, num_regs;
> +	struct cs_timing *cst;
>  
>  	if (WARN_ON(devtype->cs_regs_count > MAX_CS_REGS_COUNT))
>  		return -EINVAL;
> @@ -145,10 +158,23 @@ static int __init weim_timing_setup(struct device_node *np, void __iomem *base,
>  		if (cs_idx >= devtype->cs_count)
>  			return -EINVAL;
>  
> +		/* prevent re-configuring a CS that's already been configured */
> +		cst = &ts->cs[cs_idx];
> +		if (cst->is_applied && memcmp(value, cst->regs,
> +					devtype->cs_regs_count*sizeof(u32))) {

There should be space around *.

> +			dev_err(dev, "fsl,weim-cs-timing conflict on %pOF", np);
> +			return -EINVAL;
> +		}
> +
>  		/* set the timing for WEIM */
>  		for (i = 0; i < devtype->cs_regs_count; i++)
>  			writel(value[i],
>  				base + cs_idx * devtype->cs_stride + i * 4);
> +		if (!cst->is_applied) {
> +			cst->is_applied = true;
> +			memcpy(cst->regs, value,
> +				devtype->cs_regs_count*sizeof(u32));

Ditto.

Shawn

> +		}
>  	}
>  
>  	return 0;
> @@ -162,6 +188,7 @@ static int __init weim_parse_dt(struct platform_device *pdev,
>  	const struct imx_weim_devtype *devtype = of_id->data;
>  	struct device_node *child;
>  	int ret, have_child = 0;
> +	struct cs_timing_state ts = {};
>  
>  	if (devtype == &imx50_weim_devtype) {
>  		ret = imx_weim_gpr_setup(pdev);
> @@ -170,7 +197,7 @@ static int __init weim_parse_dt(struct platform_device *pdev,
>  	}
>  
>  	for_each_available_child_of_node(pdev->dev.of_node, child) {
> -		ret = weim_timing_setup(child, base, devtype);
> +		ret = weim_timing_setup(&pdev->dev, child, base, devtype, &ts);
>  		if (ret)
>  			dev_warn(&pdev->dev, "%pOF set timing failed.\n",
>  				child);
> -- 
> 2.17.1
>