* [PATCH v2 0/2] bus: imx-weim @ 2018-11-30 20:56 thesven73 2018-11-30 20:56 ` [PATCH v2 1/2] bus: imx-weim: support multiple address ranges per child node thesven73 2018-11-30 20:56 ` [PATCH v2 2/2] bus: imx-weim: guard against timing configuration conflicts thesven73 0 siblings, 2 replies; 6+ messages in thread From: thesven73 @ 2018-11-30 20:56 UTC (permalink / raw) To: TheSven73, Shawn Guo, Kees Cook, Rob Herring, Arnd Bergmann; +Cc: linux-kernel From: Sven Van Asbroeck <TheSven73@googlemail.com> 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] bus: imx-weim: support multiple address ranges per child node 2018-11-30 20:56 [PATCH v2 0/2] bus: imx-weim thesven73 @ 2018-11-30 20:56 ` thesven73 2018-12-05 7:51 ` Shawn Guo 2018-11-30 20:56 ` [PATCH v2 2/2] bus: imx-weim: guard against timing configuration conflicts thesven73 1 sibling, 1 reply; 6+ messages in thread From: thesven73 @ 2018-11-30 20:56 UTC (permalink / raw) To: TheSven73, Shawn Guo, Kees Cook, Rob Herring, Arnd Bergmann; +Cc: linux-kernel From: Sven Van Asbroeck <TheSven73@googlemail.com> 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 <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 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] bus: imx-weim: support multiple address ranges per child node 2018-11-30 20:56 ` [PATCH v2 1/2] bus: imx-weim: support multiple address ranges per child node thesven73 @ 2018-12-05 7:51 ` Shawn Guo 2018-12-05 15:08 ` Sven Van Asbroeck 0 siblings, 1 reply; 6+ messages in thread From: Shawn Guo @ 2018-12-05 7:51 UTC (permalink / raw) To: thesven73; +Cc: TheSven73, Kees Cook, Rob Herring, Arnd Bergmann, linux-kernel On Fri, Nov 30, 2018 at 03:56:23PM -0500, thesven73@gmail.com wrote: > From: Sven Van Asbroeck <TheSven73@googlemail.com> > > 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 <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 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] bus: imx-weim: support multiple address ranges per child node 2018-12-05 7:51 ` Shawn Guo @ 2018-12-05 15:08 ` Sven Van Asbroeck 2018-12-06 1:16 ` Shawn Guo 0 siblings, 1 reply; 6+ messages in thread From: Sven Van Asbroeck @ 2018-12-05 15:08 UTC (permalink / raw) To: Shawn Guo Cc: Kees Cook, Rob Herring, Arnd Bergmann, Linux Kernel Mailing List, devicetree Hello Shawn, many thanks for the patch review, I really appreciate it ! On Wed, Dec 5, 2018 at 2:52 AM Shawn Guo <shawnguo@kernel.org> wrote: > > On Fri, Nov 30, 2018 at 03:56:23PM -0500, thesven73@gmail.com wrote: > > From: Sven Van Asbroeck <TheSven73@googlemail.com> > > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] bus: imx-weim: support multiple address ranges per child node 2018-12-05 15:08 ` Sven Van Asbroeck @ 2018-12-06 1:16 ` Shawn Guo 0 siblings, 0 replies; 6+ messages in thread From: Shawn Guo @ 2018-12-06 1:16 UTC (permalink / raw) To: Sven Van Asbroeck Cc: Kees Cook, Rob Herring, Arnd Bergmann, Linux Kernel Mailing List, devicetree 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 <shawnguo@kernel.org> wrote: > > > > On Fri, Nov 30, 2018 at 03:56:23PM -0500, thesven73@gmail.com wrote: > > > From: Sven Van Asbroeck <TheSven73@googlemail.com> > > > > > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] bus: imx-weim: guard against timing configuration conflicts 2018-11-30 20:56 [PATCH v2 0/2] bus: imx-weim thesven73 2018-11-30 20:56 ` [PATCH v2 1/2] bus: imx-weim: support multiple address ranges per child node thesven73 @ 2018-11-30 20:56 ` thesven73 1 sibling, 0 replies; 6+ messages in thread From: thesven73 @ 2018-11-30 20:56 UTC (permalink / raw) To: TheSven73, Shawn Guo, Kees Cook, Rob Herring, Arnd Bergmann; +Cc: linux-kernel From: Sven Van Asbroeck <TheSven73@googlemail.com> 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 <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 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-12-06 1:16 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-11-30 20:56 [PATCH v2 0/2] bus: imx-weim thesven73 2018-11-30 20:56 ` [PATCH v2 1/2] bus: imx-weim: support multiple address ranges per child node thesven73 2018-12-05 7:51 ` Shawn Guo 2018-12-05 15:08 ` Sven Van Asbroeck 2018-12-06 1:16 ` Shawn Guo 2018-11-30 20:56 ` [PATCH v2 2/2] bus: imx-weim: guard against timing configuration conflicts thesven73
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.