| Message ID | 1381297324-19006-1-git-send-email-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Oct 9, 2013 at 7:42 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > It's naturally expected that when there are multiple phandle entries > specified in pinctrl-* property, these entries will be applied > sequentially. And this is how Linux kernel works. So let's define > this behavior in the binding doc. NAK this this a Linux kernel implementation detail. Sherman Yin fixed this up so the drivers does not have to imply any sequence semantic for this. Study commit 03b054e9696c3cbd3d5905ec96da15acd0a2fe8d "pinctrl: Pass all configs to driver on pin_config_set()" It is perfectly legal for a driver to accumulate the settings into e.g. a single register write if it can, e.g.: u32 val = 0; for (i = 0; i < num_configs; i++) { switch() { FOO: val |= 1; break; BAR: val |= 2: break; BAZ: val |=4; break; }; writel(val, base+pinreg); I.e. the driver may choose to apply configs sequentially, but that is not at all necessary. Yours, Linus Walleij
On Wed, Oct 09, 2013 at 02:40:49PM +0200, Linus Walleij wrote: > NAK this this a Linux kernel implementation detail. Sherman Yin > fixed this up so the drivers does not have to imply any sequence > semantic for this. > > Study commit 03b054e9696c3cbd3d5905ec96da15acd0a2fe8d > "pinctrl: Pass all configs to driver on pin_config_set()" > > It is perfectly legal for a driver to accumulate the settings into > e.g. a single register write if it can, e.g.: > > u32 val = 0; > > for (i = 0; i < num_configs; i++) { > switch() { > FOO: > val |= 1; > break; > BAR: > val |= 2: > break; > BAZ: > val |=4; > break; > }; > > writel(val, base+pinreg); > > I.e. the driver may choose to apply configs sequentially, but that is > not at all necessary. So, just to be clear, what you're saying is that specifying two settings in a pinctrl declaration which provide different values results in undefined behaviour?
On Wed, Oct 9, 2013 at 2:44 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Oct 09, 2013 at 02:40:49PM +0200, Linus Walleij wrote: >> NAK this this a Linux kernel implementation detail. Sherman Yin >> fixed this up so the drivers does not have to imply any sequence >> semantic for this. >> >> Study commit 03b054e9696c3cbd3d5905ec96da15acd0a2fe8d >> "pinctrl: Pass all configs to driver on pin_config_set()" >> >> It is perfectly legal for a driver to accumulate the settings into >> e.g. a single register write if it can, e.g.: >> >> u32 val = 0; >> >> for (i = 0; i < num_configs; i++) { >> switch() { >> FOO: >> val |= 1; >> break; >> BAR: >> val |= 2: >> break; >> BAZ: >> val |=4; >> break; >> }; >> >> writel(val, base+pinreg); >> >> I.e. the driver may choose to apply configs sequentially, but that is >> not at all necessary. > > So, just to be clear, what you're saying is that specifying two settings > in a pinctrl declaration which provide different values results in > undefined behaviour? It's more like the pin control core is passing the array of settings to the driver and the behaviour is specified per-driver. So that is from the kernels point of view, no matter whether device tree is used or not. A specific driver may instill specific behaviour - sequential or not. Sherman's driver for the Broadcom Capri had the requirement that the configuration of all parameters be done with a single register write to avoid side-effects. So it needs to iterate the configs and build up a mask and value and write it in one go. When I look at the i.MX driver (I guess this is what Shawn is working on here) it can actually be augmented to do it the same way and avoid this mess altogether: for (i = 0; i < num_configs; i++) { if (info->flags & SHARE_MUX_CONF_REG) { u32 reg; reg = readl(ipctl->base + pin_reg->conf_reg); reg &= ~0xffff; reg |= configs[i]; writel(reg, ipctl->base + pin_reg->conf_reg); } else { writel(configs[i], ipctl->base + pin_reg->conf_reg); } dev_dbg(ipctl->dev, "write: offset 0x%x val 0x%lx\n", pin_reg->conf_reg, configs[i]); } /* for each config */ As can be seen it will just writel() the config into the register for each config, it seems more like there is a bug that also the else-clause here should be read-modify-write, or am I getting it all wrong? In some cases some configs may be electrically conflicting and completely unsound, like enabling pull-up and pull-down at the same time - maybe possible in the hardware but insane, so the driver should detect that state in this loop and reject such configs, print a warning or make a best effort. I am aware that maybe not all drivers are handling this in the most ultimate way today :-/ Then as this patch was against the device tree documentation: since the device tree isn't really about sequencing things (Grant has been onto us in the past for wanting to put sequences in there) we should probably avoid defining such things as sequences at all, as that basically moves the hardware description toward a sequence description language (jam table), and for that the full open firmware or other BIOS thing (ACPI?) should be used instead. (If I understood the point correctly.) I tend to think about the DT language as functional i.e. there is no way to say anything about sequencing order from it, I may be wrong here though. Yours, Linus Walleij
On Wed, Oct 09, 2013 at 03:09:21PM +0200, Linus Walleij wrote: > It's more like the pin control core is passing the array of settings > to the driver and the behaviour is specified per-driver. > > So that is from the kernels point of view, no matter whether > device tree is used or not. A specific driver may instill specific > behaviour - sequential or not. Right, so that means doing this: pinctrl_usdhc1_1: usdhc1grp-1 { fsl,pins = < ... MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059 ... >; }; pinctrl_usdhc1_1_dat3cd: usdhc1grp-3 { fsl,pins = < MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x13059 >; }; and then: pinctrl-0 = <&pinctrl_usdhc1_1 &pinctrl_usdhc1_1_dat3cd>; can result in either "MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059" or "MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x13059" being the final configuration for that pin. What that means is that for any pinctrl setting, pins to be configured must be mentioned exactly once and once only.
On Wed, Oct 9, 2013 at 3:45 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > Right, so that means doing this: > > pinctrl_usdhc1_1: usdhc1grp-1 { > fsl,pins = < > ... > MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059 > ... > >; > }; > > pinctrl_usdhc1_1_dat3cd: usdhc1grp-3 { > fsl,pins = < > MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x13059 > >; > }; > > and then: > > pinctrl-0 = <&pinctrl_usdhc1_1 &pinctrl_usdhc1_1_dat3cd>; > > can result in either "MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059" or > "MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x13059" being the final configuration > for that pin. I'm uncertain about the device tree case. I guess it is indeed an array of handles indexed [0,1], the framework will only send that array down to the driver, no strings attached. > What that means is that for any pinctrl setting, pins to be configured > must be mentioned exactly once and once only. Either that (and the above does seem very ambigous) or the driver need to combine and handle the conflicting configurations, maintaining a state for the pin and so on ... This seems like two *complete* configs fighting over the same pin rather than two complementary configs that we could | together and write, and that is indeed ambigous. The array passed to the config function is supposed to be individual settings like [ pull-up, schmitt-trigger ] not two complete sets of config. The generic pin config binding is luckily more clear than this i.MX-specific handling. Yours, Linus Walleij
On 10/09/2013 06:44 AM, Russell King - ARM Linux wrote: > On Wed, Oct 09, 2013 at 02:40:49PM +0200, Linus Walleij wrote: >> NAK this this a Linux kernel implementation detail. Sherman Yin >> fixed this up so the drivers does not have to imply any sequence >> semantic for this. >> >> Study commit 03b054e9696c3cbd3d5905ec96da15acd0a2fe8d >> "pinctrl: Pass all configs to driver on pin_config_set()" >> >> It is perfectly legal for a driver to accumulate the settings into >> e.g. a single register write if it can, e.g.: >> >> u32 val = 0; >> >> for (i = 0; i < num_configs; i++) { >> switch() { >> FOO: >> val |= 1; >> break; >> BAR: >> val |= 2: >> break; >> BAZ: >> val |=4; >> break; >> }; >> >> writel(val, base+pinreg); >> >> I.e. the driver may choose to apply configs sequentially, but that is >> not at all necessary. > > So, just to be clear, what you're saying is that specifying two settings > in a pinctrl declaration which provide different values results in > undefined behaviour? That makes sense to me, yes. It should be simple to separate out the common/shared parts of a configuration, vs. other unique parts, and put them into separate "pin configuration nodes", and hence avoid the situation completely.
On Wed, Oct 09, 2013 at 02:40:49PM +0200, Linus Walleij wrote: > On Wed, Oct 9, 2013 at 7:42 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > > It's naturally expected that when there are multiple phandle entries > > specified in pinctrl-* property, these entries will be applied > > sequentially. And this is how Linux kernel works. So let's define > > this behavior in the binding doc. > > NAK this this a Linux kernel implementation detail. Sherman Yin > fixed this up so the drivers does not have to imply any sequence > semantic for this. > > Study commit 03b054e9696c3cbd3d5905ec96da15acd0a2fe8d > "pinctrl: Pass all configs to driver on pin_config_set()" > > It is perfectly legal for a driver to accumulate the settings into > e.g. a single register write if it can, e.g.: > > u32 val = 0; > > for (i = 0; i < num_configs; i++) { > switch() { > FOO: > val |= 1; > break; > BAR: > val |= 2: > break; > BAZ: > val |=4; > break; > }; > > writel(val, base+pinreg); > > I.e. the driver may choose to apply configs sequentially, but that is > not at all necessary. Yes, that's the case when pinconf_apply_setting() is called to apply a struct pinctrl_setting. This pinctrl_setting may contain multiple configs (pull, drive, open drain etc.) for a pin, and driver may choose to apply these configs sequentially or accumulatedly. In case of device tree, all these configs must be coming from a single pin configuration node, as per my understanding of how pinctrl core code maps pin configuration in device tree node to struct pinctrl_setting. However, my patch is talking about a different thing. For example, we have a device whose pinctrl-0 consists of two phandle entries that point to pin configuration nodes foo and bar. pinctrl-0 = <&foo &bar>; foo { ... }; bar { ... }; My patch only wants to make it clear that the configuration specified by node foo will be applied to pin controller first, and the configuration defined in node bar will be applied after that. When both nodes have configuration for a pin, these two configs for the same pin go to two different pinctrl_setting structures. And these two pinctrl_settings can not be applied accumulatedly but only sequentially. That's what my patch talks about. Shawn
On Thu, Oct 10, 2013 at 9:26 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > However, my patch is talking about a different thing. For example, we > have a device whose pinctrl-0 consists of two phandle entries that point > to pin configuration nodes foo and bar. > > pinctrl-0 = <&foo &bar>; > > foo { > ... > }; > > bar { > ... > }; > > My patch only wants to make it clear that the configuration specified by > node foo will be applied to pin controller first, and the configuration > defined in node bar will be applied after that. When both nodes have > configuration for a pin, these two configs for the same pin go to two > different pinctrl_setting structures. And these two pinctrl_settings > can not be applied accumulatedly but only sequentially. That's what my > patch talks about. OK I get it (I think). However isn't this aspect rather a property of how the specific driver parses and uses the device tree? Remember that with drivers that do not use generic pin config, we have left the handling of the device tree node up to the driver by the callback dt_node_to_map(). But I do understand the ambition to specify this behaviour for all pin control drivers using device tree. But we need to be careful since DT isn't very much in the buisiness of defining sequence semantics for stuff, and that is why I'm a bit hesitant. So, maybe we can add this as it only concerns the binding (but then I want that example to be part of the documentation patch so that it is crystal clear what we're talking about), but it would be proper if we also helped enforce this semantic across the drivers, i.e. centralize more of the DT parsing code... BTW on a related subject: have you examined if the i.MX driver can use the new utility functions from pinctrl-utils.h: pinctrl_utils_reserve_map() pinctrl_utils_add_map_mux() pinctrl_utils_add_map_configs() pinctrl_utils_add_config() pinctrl_utils_dt_free_map() It helps a lot the more boilerplate we can move out of the drivers and get a clear picture of the device tree parsing code, and see what really needs to be different across the drivers. Yours, Linus Walleij
On Wed, Oct 09, 2013 at 03:09:21PM +0200, Linus Walleij wrote: > When I look at the i.MX driver (I guess this is what Shawn > is working on here) it can actually be augmented to do it > the same way and avoid this mess altogether: > > for (i = 0; i < num_configs; i++) { > if (info->flags & SHARE_MUX_CONF_REG) { > u32 reg; > reg = readl(ipctl->base + pin_reg->conf_reg); > reg &= ~0xffff; > reg |= configs[i]; > writel(reg, ipctl->base + pin_reg->conf_reg); > } else { > writel(configs[i], ipctl->base + pin_reg->conf_reg); > } > dev_dbg(ipctl->dev, "write: offset 0x%x val 0x%lx\n", > pin_reg->conf_reg, configs[i]); > } /* for each config */ > > As can be seen it will just writel() the config into the register > for each config, it seems more like there is a bug that also > the else-clause here should be read-modify-write, or am > I getting it all wrong? On imx, num_configs is always 1 (new_map[j].data.configs.num_configs is hard-coded as 1 in imx_dt_node_to_map() function). We have all configs for given pin encoded in the last integer of fsl,pin entry. And this integer can be directly written into conf_reg. Shawn
On Thu, Oct 10, 2013 at 03:26:26PM +0800, Shawn Guo wrote: > However, my patch is talking about a different thing. For example, we > have a device whose pinctrl-0 consists of two phandle entries that point > to pin configuration nodes foo and bar. > > pinctrl-0 = <&foo &bar>; > > foo { > ... > }; > > bar { > ... > }; > > My patch only wants to make it clear that the configuration specified by > node foo will be applied to pin controller first, and the configuration > defined in node bar will be applied after that. When both nodes have > configuration for a pin, these two configs for the same pin go to two > different pinctrl_setting structures. And these two pinctrl_settings > can not be applied accumulatedly but only sequentially. That's what my > patch talks about. I still say this is a potentially dangerous thing, and in my case of overriding the DAT3 pull-sense, it will cause the pin to glitch if nothing is connected to it. So even if you do get this clarified, I am *not* happy to change my patch.
On Thu, Oct 10, 2013 at 10:03:40AM +0200, Linus Walleij wrote: > On Thu, Oct 10, 2013 at 9:26 AM, Shawn Guo <shawn.guo@linaro.org> wrote: > > > However, my patch is talking about a different thing. For example, we > > have a device whose pinctrl-0 consists of two phandle entries that point > > to pin configuration nodes foo and bar. > > > > pinctrl-0 = <&foo &bar>; > > > > foo { > > ... > > }; > > > > bar { > > ... > > }; > > > > My patch only wants to make it clear that the configuration specified by > > node foo will be applied to pin controller first, and the configuration > > defined in node bar will be applied after that. When both nodes have > > configuration for a pin, these two configs for the same pin go to two > > different pinctrl_setting structures. And these two pinctrl_settings > > can not be applied accumulatedly but only sequentially. That's what my > > patch talks about. > > OK I get it (I think). > > However isn't this aspect rather a property of how the specific driver > parses and uses the device tree? The 'pinctrl-0' property is defined by common pinctrl device tree binding and is parsed by pinctrl core, while pin configuration node is platform specific and handled by controller driver. > Remember that with drivers that do not use generic pin config, > we have left the handling of the device tree node up to the driver > by the callback dt_node_to_map(). Yes. dt_node_to_map() takes pin configuration node pointer as input and return a set of struct pinctrl_map. But how pinctrl_map eventually gets turned into pinctrl_setting as the input to pinconf_apply_setting() call are all handled by pinctrl core and common across different controller drivers. > But I do understand the ambition to specify this behaviour for all > pin control drivers using device tree. > > But we need to be careful since DT isn't very much in the > buisiness of defining sequence semantics for stuff, and that > is why I'm a bit hesitant. > > So, maybe we can add this as it only concerns the binding > (but then I want that example to be part of the documentation patch > so that it is crystal clear what we're talking about), but it would be > proper if we also helped enforce this semantic across the drivers, > i.e. centralize more of the DT parsing code... Ok. I would hold it off for a while and consider Stephen's comment first for solving the problem we have on i.MX. > BTW on a related subject: have you examined if the i.MX driver > can use the new utility functions from pinctrl-utils.h: > > pinctrl_utils_reserve_map() > pinctrl_utils_add_map_mux() > pinctrl_utils_add_map_configs() > pinctrl_utils_add_config() > pinctrl_utils_dt_free_map() Thanks for the info. Just had a quick look at them, and it seems that they don't save too much code for imx. Will take another deep look though. Shawn
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index 1958ca9..404ba32 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt @@ -50,7 +50,13 @@ pinctrl-0: List of phandles, each pointing at a pin configuration entries may exist in this list so that multiple pin controllers may be configured, or so that a state may be built from multiple nodes for a single pin controller, each - contributing part of the overall configuration. See the next + contributing part of the overall configuration. These entries + will be applied sequentially. If there are multiple entries + contributing the configuration of the same pin, the latter + will overwrite the former. However, this 'overwrite' mechanism + should be used with the caution that it could cause some ill + effect, e.g. a glitch on the pin when pull down/up setting + gets flipped in this 'overwrite'. See the next section of this document for details of the format of these pin configuration nodes.
It's naturally expected that when there are multiple phandle entries specified in pinctrl-* property, these entries will be applied sequentially. And this is how Linux kernel works. So let's define this behavior in the binding doc. The behavior is useful when people want to reuse a group of predefined pins with only minor configuration adjustment on one particular pin. They will only need to add another entry after the predefined one with needed configuration on that particular pin to overwrite the predefined configuration. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- .../bindings/pinctrl/pinctrl-bindings.txt | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)