bus: imx-weim
Subsystems (2)
v3 cover bus: imx-weim
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(-)
Signature · 1 line
2.17.1
PATCH v3 1/3 bus: imx-weim: support multiple address ranges per child node
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
@@ -46,6 +46,7 @@ static const struct imx_weim_devtype imx51_weim_devtype = { };#define MAX_CS_REGS_COUNT 6 +#define OF_REG_SIZE 3static 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
Re: [PATCH v3 1/3] bus: imx-weim: support multiple address ranges per child node
Quoted 35 lines · 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 <[email protected]>--- drivers/bus/imx-weim.c | 36 +++++++++++++++++++++++++----------- 1 file changed, 25 insertions(+), 11 deletions(-)drivers/bus/imx-weim.c +1
-0@@ -46,6 +46,7 @@ static const struct imx_weim_devtype imx51_weim_devtype = { };#define MAX_CS_REGS_COUNT 6 +#define OF_REG_SIZE 3static const struct of_device_id weim_id_table[] = {/* i.MX1/21 */
drivers/bus/imx-weim.c static int __init weim_timing_setup(struct device_node *np, void __iomem *base, · line 119
Quoted 5 lines
drivers/bus/imx-weim.c +1
-1@@ -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;
Quoted 15 lines
drivers/bus/imx-weim.c +5
-3if (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.
drivers/bus/imx-weim.c static int __init weim_timing_setup(struct device_node *np, void __iomem *base, · line 141
Quoted 10 lines
drivers/bus/imx-weim.c +9
-0+*/ + 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
Quoted 24 lines
drivers/bus/imx-weim.c +9
-7+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
PATCH v3 2/3 dt-bindings: bus: imx-weim: document multiple address ranges per child node
In reply to Sven Van AsbroeckThe 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.
--- .../devicetree/bindings/bus/imx-weim.txt | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/bus/imx-weim.txt +29 -3
@@ -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>; + }; + };
Signature · 1 line
2.17.1
Re: [PATCH v3 2/3] dt-bindings: bus: imx-weim: document multiple address ranges per child node
Quoted 9 lines · 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 <[email protected]>--- .../devicetree/bindings/bus/imx-weim.txt | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-)
Re: [PATCH v3 2/3] dt-bindings: bus: imx-weim: document multiple address ranges per child node
In reply to Sven Van AsbroeckQuoted 5 lines · 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 <[email protected]>
The bindings patch should go first in the series.
Shawn
PATCH v3 3/3 bus: imx-weim: guard against timing configuration conflicts
In reply to Sven Van AsbroeckWhen 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>;
};
};--- drivers/bus/imx-weim.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-)
drivers/bus/imx-weim.c +30 -3
@@ -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);
Signature · 1 line
2.17.1
Re: [PATCH v3 3/3] bus: imx-weim: guard against timing configuration conflicts
Quoted 49 lines · 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 <[email protected]>--- drivers/bus/imx-weim.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-)drivers/bus/imx-weim.c +0
-0
drivers/bus/imx-weim.c static const struct imx_weim_devtype imx51_weim_devtype = { · line 49
Quoted 4 lines
drivers/bus/imx-weim.c +1
-0@@ -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?
Quoted 32 lines
drivers/bus/imx-weim.c +14
-2#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 ;
drivers/bus/imx-weim.c static int __init weim_timing_setup(struct device_node *np, void __iomem *base, · line 164
Quoted 7 lines
drivers/bus/imx-weim.c +4
-0@@ -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 *.
drivers/bus/imx-weim.c static int __init weim_timing_setup(struct device_node *np, void __iomem *base, · line 176
Quoted 12 lines
drivers/bus/imx-weim.c +8
-0+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
Quoted 24 lines
drivers/bus/imx-weim.c +3
-1+ } }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);Signature · 1 line
2.17.1