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.
v2:
corrected acme@... in commit message example
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(-)
--
2.17.1
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,0 {
compatible = "acme,whatever";
reg = <0 0 0x100>;
fsl,weim-cs-timing = <something>;
};
acme@0,500 {
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,0 {
compatible = "acme,whatever";
reg = <0 0 0x100>;
fsl,weim-cs-timing = <something>;
};
acme@0,500 {
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(-)
diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 24f22285395d..114e503ec785 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;
@@ -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
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,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(-)
diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index f01308172de9..24f22285395d 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
On Fri, Nov 30, 2018 at 03:56:23PM -0500, [email protected] wrote:
> 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,0 {
> compatible = "acme,whatever";
> reg = <0 0 0x100>, <0 0x400000 0x800>,
> <1 0x400000 0x800>;
> fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
> 0x00000000 0xa0000240 0x00000000>;
> };
> };
I'm not sure about that. Shouldn't we have another child node for
different chip select, something like below?
&weim {
acme@0,0 {
compatible = "acme,whatever";
reg = <0 0 0x100>, <0 0x400000 0x800>;
fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
0x00000000 0xa0000240 0x00000000>;
};
acme@1,400000 {
compatible = "acme,whatever";
reg = <1 0x400000 0x800>;
fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
0x00000000 0xa0000240 0x00000000>;
};
Shawn
>
> Signed-off-by: Sven Van Asbroeck <[email protected]>
> ---
> 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 f01308172de9..24f22285395d 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
>
Hello Shawn, many thanks for the patch review, I really appreciate it !
On Wed, Dec 5, 2018 at 2:52 AM Shawn Guo <[email protected]> wrote:
>
> On Fri, Nov 30, 2018 at 03:56:23PM -0500, [email protected] wrote:
> > 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.
> >
>
> I'm not sure about that. Shouldn't we have another child node for
> different chip select, something like below?
>
> &weim {
> acme@0,0 {
> compatible = "acme,whatever";
> reg = <0 0 0x100>, <0 0x400000 0x800>;
> fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
> 0x00000000 0xa0000240 0x00000000>;
> };
>
> acme@1,400000 {
> compatible = "acme,whatever";
> reg = <1 0x400000 0x800>;
> fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
> 0x00000000 0xa0000240 0x00000000>;
> };
>
> Shawn
I am submitting patches for a device that spans chip selects :(
And such a device needs multiple address changes with different chip selects.
Imagine we have an acme device, which contains a control and a fifo region,
on different chip selects:
&weim {
acme@0 {
compatible = "acme";
reg = <0 0x0 0x100>, <1 0x0 0x100>;
};
};
Now in probe we can access both regions:
int acme_probe(struct platform_device *pdev)
{
control_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
fifo_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
/* all ok */
}
But, if we have two separate child nodes, we also get two calls to probe(),
which assumes two devices on the bus, and that is incorrect:
&weim {
acme@0 {
compatible = "acme";
reg = <0 0x0 0x100>;
};
acme@1 {
compatible = "acme";
reg = <1 0x0 0x100>;
};
};
int acme_probe(struct platform_device *pdev)
{
control_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
/* next call always fails */
fifo_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
}
For my patchset, Rob Herring suggested I made changes to the imx-weim driver
to accommodate multi-chipselect devices.
See the conversation below between Rob Herring and myself:
https://lkml.org/lkml/2018/11/30/390
If you are not entirely satisfied with my patch, then perhaps you
could think of another way to support multi-chipselect devices?
Many thanks,
Sven
On Wed, Dec 05, 2018 at 10:08:16AM -0500, Sven Van Asbroeck wrote:
> Hello Shawn, many thanks for the patch review, I really appreciate it !
>
> On Wed, Dec 5, 2018 at 2:52 AM Shawn Guo <[email protected]> wrote:
> >
> > On Fri, Nov 30, 2018 at 03:56:23PM -0500, [email protected] wrote:
> > > 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.
> > >
> >
> > I'm not sure about that. Shouldn't we have another child node for
> > different chip select, something like below?
> >
> > &weim {
> > acme@0,0 {
> > compatible = "acme,whatever";
> > reg = <0 0 0x100>, <0 0x400000 0x800>;
> > fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
> > 0x00000000 0xa0000240 0x00000000>;
> > };
> >
> > acme@1,400000 {
> > compatible = "acme,whatever";
> > reg = <1 0x400000 0x800>;
> > fsl,weim-cs-timing = <0x024400b1 0x00001010 0x20081100
> > 0x00000000 0xa0000240 0x00000000>;
> > };
> >
> > Shawn
>
> I am submitting patches for a device that spans chip selects :(
> And such a device needs multiple address changes with different chip selects.
>
> Imagine we have an acme device, which contains a control and a fifo region,
> on different chip selects:
>
> &weim {
> acme@0 {
> compatible = "acme";
> reg = <0 0x0 0x100>, <1 0x0 0x100>;
> };
> };
>
> Now in probe we can access both regions:
> int acme_probe(struct platform_device *pdev)
> {
> control_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> fifo_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> /* all ok */
> }
>
> But, if we have two separate child nodes, we also get two calls to probe(),
> which assumes two devices on the bus, and that is incorrect:
>
> &weim {
> acme@0 {
> compatible = "acme";
> reg = <0 0x0 0x100>;
> };
> acme@1 {
> compatible = "acme";
> reg = <1 0x0 0x100>;
> };
> };
>
> int acme_probe(struct platform_device *pdev)
> {
> control_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> /* next call always fails */
> fifo_res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> }
>
> For my patchset, Rob Herring suggested I made changes to the imx-weim driver
> to accommodate multi-chipselect devices.
>
> See the conversation below between Rob Herring and myself:
> https://lkml.org/lkml/2018/11/30/390
Sorry, I wasn't aware of the discussion. Now I understand the
background of the changes. But can you please patch imx-weim bindings
doc (Documentation/devicetree/bindings/bus/imx-weim.txt) to accommodate
this new use scenario?
Shawn