Skip to contentSkip to conversation

bus: imx-weim

PATCH Superseded [email protected] 2 participants 4 messages Nov 30, 2018 Arm i.MX and Layerscape

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

cover bus: imx-weim

From: Sven Van Asbroeck <[email protected]>

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

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

 drivers/bus/imx-weim.c | 69 +++++++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 14 deletions(-)
Signature · 1 line
2.17.1
#1 lore raw

PATCH 1/2 bus: imx-weim: support multiple address ranges per child node

From: Sven Van Asbroeck <[email protected]>

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>;
	};
};
---
 drivers/bus/imx-weim.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)
drivers/bus/imx-weim.c +25 -11
index f01308172de9..24f22285395d 100644
@@ -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;
 }
Signature · 1 line
2.17.1
#2 lore raw

PATCH 2/2 bus: imx-weim: guard against timing configuration conflicts

In reply to [email protected]

From: Sven Van Asbroeck <[email protected]>

When adding weim child devices, there is a risk that the developer will (by mistake) specify more than one timing setting 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>;
        };
};
---
 drivers/bus/imx-weim.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)
drivers/bus/imx-weim.c +30 -3
index 24f22285395d..114e503ec785 100644
@@ -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;
@@ -165,6 +191,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);
@@ -179,7 +206,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);
Signature · 1 line
2.17.1
#3 lore raw

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

Quoted 8 lines · On Fri, Nov 30, 2018 at 11:08 AM <[email protected]> wrote:

From: Sven Van Asbroeck <[email protected]>

When adding weim child devices, there is a risk that the developer will (by mistake) specify more than one timing setting for the same chip select.

The driver cannot support such a configuration.

Quoted 8 lines

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 {

This should be '@0,0'

Quoted 5 lines
                compatible = "acme,whatever";
                reg = <0 0 0x100>;
                fsl,weim-cs-timing = <something>;
        };
        acme@1 {

And '@0,500'

Quoted 110 lines
                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>;
        };
};
---
 drivers/bus/imx-weim.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)
drivers/bus/imx-weim.c +30 -4
index 24f22285395d..114e503ec785 100644
@@ -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;
@@ -165,6 +191,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);
@@ -179,7 +206,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