| Message ID | 1433948699-19800-3-git-send-email-ludovic.desroches@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/10/2015 09:04 AM, Ludovic Desroches wrote: > Using a string to describe a pin in the device tree can be not enough. > Some controllers may need extra information to fully describe a pin. It > concerns mainly controllers which have a per pin muxing approach which > don't fit well the notions of groups and functions. > Instead of using a pin name, a 32 bit value is used. The 16 least > significant bits are used for the pin number. Other 16 bits can be used to > store extra parameters. The driver for the pin controller is supposed to provide this information in a table. The whole point of having a driver, rather than a table/list of raw register values in the DT, is so the driver can provide this information at a semantic level. This information is fixed per SoC and so make sense to put into a driver, while the board-specific configuration varies wildly, and hence makes sense to put into DT.
On Mon, Jun 15, 2015 at 10:01:29AM -0600, Stephen Warren wrote: > On 06/10/2015 09:04 AM, Ludovic Desroches wrote: > >Using a string to describe a pin in the device tree can be not enough. > >Some controllers may need extra information to fully describe a pin. It > >concerns mainly controllers which have a per pin muxing approach which > >don't fit well the notions of groups and functions. > >Instead of using a pin name, a 32 bit value is used. The 16 least > >significant bits are used for the pin number. Other 16 bits can be used to > >store extra parameters. > > The driver for the pin controller is supposed to provide this information in > a table. The whole point of having a driver, rather than a table/list of raw > register values in the DT, is so the driver can provide this information at > a semantic level. This information is fixed per SoC and so make sense to put > into a driver, while the board-specific configuration varies wildly, and > hence makes sense to put into DT. > > I didn't think the controversery part would be about having this information in a driver or in the device tree. I think there are pros and cons for both cases. We already have this description in our dt file with the previous at91 pin controller and I think it is a good thing to not have to update the driver for each new SoC using the same pio controller. Tables could become huge, embedding several one into a 'single zImage' is something I am not confortable with. I know that some people could tell me that doing that may increase the boot time. We should debate about this patch later. If there is no acceptance for the previous one, I have to clarify if I need it. Regards Ludovic
On Wed, Jun 10, 2015 at 05:04:57PM +0200, Ludovic Desroches wrote: > Using a string to describe a pin in the device tree can be not enough. > Some controllers may need extra information to fully describe a pin. It > concerns mainly controllers which have a per pin muxing approach which > don't fit well the notions of groups and functions. > Instead of using a pin name, a 32 bit value is used. The 16 least > significant bits are used for the pin number. Other 16 bits can be used to > store extra parameters. In the Mediatek driver we use 'pinmux' as name for the property containing the combined pin number / mux value defines. 'pinmux' better describes what it is... > + > + if (pctldesc->complex_pin_desc) > + ret = of_property_count_u32_elems(np, "pins"); > + else > + ret = of_property_count_strings(np, "pins"); ... and has the advantage that you don't have to pass in a complex_pin_desc variable from the driver as the different property name inherently carries this information. 'pins' can then stay a property containing only strings. > if (ret < 0) { > ret = of_property_count_strings(np, "groups"); > if (ret < 0) > @@ -297,11 +305,12 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, > if (type == PIN_MAP_TYPE_INVALID) > type = PIN_MAP_TYPE_CONFIGS_GROUP; > subnode_target_type = "groups"; > + pins_prop = false; > } else { > if (type == PIN_MAP_TYPE_INVALID) > type = PIN_MAP_TYPE_CONFIGS_PIN; > } > - strings_count = ret; > + items_count = ret; > > ret = of_property_read_string(np, "function", &function); > if (ret < 0) { > @@ -326,17 +335,31 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, > if (num_configs) > reserve++; > > - reserve *= strings_count; > + reserve *= items_count; > > ret = pinctrl_utils_reserve_map(pctldev, map, reserved_maps, > num_maps, reserve); > if (ret < 0) > goto exit; > > - of_property_for_each_string(np, subnode_target_type, prop, group) { > + items_name = kmalloc_array(items_count, sizeof(char *), GFP_KERNEL); > + if (!items_name) > + goto exit; > + if (pctldesc->complex_pin_desc && pins_prop) { > + of_property_for_each_u32(np, subnode_target_type, prop, cur, val) { > + pin_id = val & PINCTRL_PIN_MASK; > + items_name[i++] = pctldesc->pins[pin_id].name; > + } I don't like that even though pins have numbers here they are converted to strings which the driver later has to search in a list just to convert it back into the number. This is quite inefficient. I guess this could be optimized later, but it would be nice to have the pin number directly in the driver. Sascha
On Tue, Jul 14, 2015 at 08:13:59AM +0200, Sascha Hauer wrote: > On Wed, Jun 10, 2015 at 05:04:57PM +0200, Ludovic Desroches wrote: > > Using a string to describe a pin in the device tree can be not enough. > > Some controllers may need extra information to fully describe a pin. It > > concerns mainly controllers which have a per pin muxing approach which > > don't fit well the notions of groups and functions. > > Instead of using a pin name, a 32 bit value is used. The 16 least > > significant bits are used for the pin number. Other 16 bits can be used to > > store extra parameters. > > In the Mediatek driver we use 'pinmux' as name for the property > containing the combined pin number / mux value defines. 'pinmux' better > describes what it is... > At the moment, I don't mix pin number and pin mux. I mix pin number and ioset. It allows to check that all the pins belong to the same ioset. As said previously, I didn't want to mix pin mux and pin conf in the same node (but it is something I can do, it's not a problem on my side). If I do it I will have to mux three values: pin number, pin mux value and pin ioset. So assuming I do this change, your advice is to add a 'pinmux' property in addition of 'pins' instead of trying to use it? > > + > > + if (pctldesc->complex_pin_desc) > > + ret = of_property_count_u32_elems(np, "pins"); > > + else > > + ret = of_property_count_strings(np, "pins"); > > ... and has the advantage that you don't have to pass in a > complex_pin_desc variable from the driver as the different property > name inherently carries this information. 'pins' can then stay a > property containing only strings. > > > if (ret < 0) { > > ret = of_property_count_strings(np, "groups"); > > if (ret < 0) > > @@ -297,11 +305,12 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, > > if (type == PIN_MAP_TYPE_INVALID) > > type = PIN_MAP_TYPE_CONFIGS_GROUP; > > subnode_target_type = "groups"; > > + pins_prop = false; > > } else { > > if (type == PIN_MAP_TYPE_INVALID) > > type = PIN_MAP_TYPE_CONFIGS_PIN; > > } > > - strings_count = ret; > > + items_count = ret; > > > > ret = of_property_read_string(np, "function", &function); > > if (ret < 0) { > > @@ -326,17 +335,31 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, > > if (num_configs) > > reserve++; > > > > - reserve *= strings_count; > > + reserve *= items_count; > > > > ret = pinctrl_utils_reserve_map(pctldev, map, reserved_maps, > > num_maps, reserve); > > if (ret < 0) > > goto exit; > > > > - of_property_for_each_string(np, subnode_target_type, prop, group) { > > + items_name = kmalloc_array(items_count, sizeof(char *), GFP_KERNEL); > > + if (!items_name) > > + goto exit; > > + if (pctldesc->complex_pin_desc && pins_prop) { > > + of_property_for_each_u32(np, subnode_target_type, prop, cur, val) { > > + pin_id = val & PINCTRL_PIN_MASK; > > + items_name[i++] = pctldesc->pins[pin_id].name; > > + } > > I don't like that even though pins have numbers here they are converted > to strings which the driver later has to search in a list just to > convert it back into the number. This is quite inefficient. > > I guess this could be optimized later, but it would be nice to have the > pin number directly in the driver. I know that is something you don't like but, at the moment, I need a string for pinctrl_utils_add_map_mux and pinctrl_utils_add_map_configs. Ludovic
On Wed, Jul 15, 2015 at 10:45:42AM +0200, Ludovic Desroches wrote: > On Tue, Jul 14, 2015 at 08:13:59AM +0200, Sascha Hauer wrote: > > On Wed, Jun 10, 2015 at 05:04:57PM +0200, Ludovic Desroches wrote: > > > Using a string to describe a pin in the device tree can be not enough. > > > Some controllers may need extra information to fully describe a pin. It > > > concerns mainly controllers which have a per pin muxing approach which > > > don't fit well the notions of groups and functions. > > > Instead of using a pin name, a 32 bit value is used. The 16 least > > > significant bits are used for the pin number. Other 16 bits can be used to > > > store extra parameters. > > > > In the Mediatek driver we use 'pinmux' as name for the property > > containing the combined pin number / mux value defines. 'pinmux' better > > describes what it is... > > > > At the moment, I don't mix pin number and pin mux. I mix pin number and > ioset. It allows to check that all the pins belong to the same ioset. > > As said previously, I didn't want to mix pin mux and pin conf in the > same node (but it is something I can do, it's not a problem on my side). > If I do it I will have to mux three values: pin number, pin mux value > and pin ioset. > > So assuming I do this change, your advice is to add a 'pinmux' property in > addition of 'pins' instead of trying to use it? My advise is to not enslave your code to this ioset concept. The only effect of introducing this concept is one single warning in the log: dev_warn(&pdev->dev, "/!\\ pins from group %s are not using the same ioset /!\\\n", group->name); There are *no* decisions made in the driver upon the ioset, only the above warning is printed. I can easily find examples in which a device needs some functional pins and some additional GPIOs, be it for card detection or something else, and this *will* work, regardless of which ioset the pins are in. Why should a ioset concept limit me in this way that everything works except I have to suppress or ignore the warning in the log or split the pinmux node up to two different nodes? The only thing that won't work (or at least you don't want to guarantee that it works), is to mix functional pins from the upper left corner of the SoC with pins from the lower right corner to form a single MMC/SD controller. Yes, you shouldn't do that. I may or may not work, but that is nothing this particular SoC introduces, it's like this on many other SoCs, perhaps even including earlier Atmel SoCs. Still, me as the guy writing board support have to support the way the hardware is designed, and if a board designer chooses to use pins from different iosets for a single device I'll support it that way, no matter if a warning is shown or not. If the users of this board are annoyed by the above warning you'll probably receive a patch dropping it soon. Then the ioset concept has completely vanished, but still the device trees are written like that. So, yes, my advice is to drop the ioset concept completely. If you still insist on it then you can encode the ioset number in the pinmux define. Only the lower 16bit are defined as the pin number, you can use the upper 16bit like you want. You can encode the muxing in bits 16-23 and the ioset in bits 24-31. > > > + if (pctldesc->complex_pin_desc && pins_prop) { > > > + of_property_for_each_u32(np, subnode_target_type, prop, cur, val) { > > > + pin_id = val & PINCTRL_PIN_MASK; > > > + items_name[i++] = pctldesc->pins[pin_id].name; > > > + } > > > > I don't like that even though pins have numbers here they are converted > > to strings which the driver later has to search in a list just to > > convert it back into the number. This is quite inefficient. > > > > I guess this could be optimized later, but it would be nice to have the > > pin number directly in the driver. > > I know that is something you don't like but, at the moment, I need a string for > pinctrl_utils_add_map_mux and pinctrl_utils_add_map_configs. Yeah, I am fine with it. This can be fixed later. I am more concerned about the device tree bindings than about the actual implementation. The implementation can far more easy be changed than the bindings. Sascha
On Wed, Jul 15, 2015 at 12:05:25PM +0200, Sascha Hauer wrote: > On Wed, Jul 15, 2015 at 10:45:42AM +0200, Ludovic Desroches wrote: > > On Tue, Jul 14, 2015 at 08:13:59AM +0200, Sascha Hauer wrote: > > > On Wed, Jun 10, 2015 at 05:04:57PM +0200, Ludovic Desroches wrote: > > > > Using a string to describe a pin in the device tree can be not enough. > > > > Some controllers may need extra information to fully describe a pin. It > > > > concerns mainly controllers which have a per pin muxing approach which > > > > don't fit well the notions of groups and functions. > > > > Instead of using a pin name, a 32 bit value is used. The 16 least > > > > significant bits are used for the pin number. Other 16 bits can be used to > > > > store extra parameters. > > > > > > In the Mediatek driver we use 'pinmux' as name for the property > > > containing the combined pin number / mux value defines. 'pinmux' better > > > describes what it is... > > > > > > > At the moment, I don't mix pin number and pin mux. I mix pin number and > > ioset. It allows to check that all the pins belong to the same ioset. > > > > As said previously, I didn't want to mix pin mux and pin conf in the > > same node (but it is something I can do, it's not a problem on my side). > > If I do it I will have to mux three values: pin number, pin mux value > > and pin ioset. > > > > So assuming I do this change, your advice is to add a 'pinmux' property in > > addition of 'pins' instead of trying to use it? > > My advise is to not enslave your code to this ioset concept. The only > effect of introducing this concept is one single warning in the log: My code is not totally enslaved to the ioset concept. The group_defs node is not only for ioset, it is also to have comprehensive group names when consultind sysfs. I don't know how I can achieve this if I remove group_defs... Maybe with the parent node name? > > dev_warn(&pdev->dev, > "/!\\ pins from group %s are not using the same ioset /!\\\n", > group->name); > > There are *no* decisions made in the driver upon the ioset, only the > above warning is printed. > > I can easily find examples in which a device needs some functional pins > and some additional GPIOs, be it for card detection or something else, > and this *will* work, regardless of which ioset the pins are in. Why > should a ioset concept limit me in this way that everything works except > I have to suppress or ignore the warning in the log or split the pinmux > node up to two different nodes? > > The only thing that won't work (or at least you don't want to guarantee > that it works), is to mix functional pins from the upper left corner of > the SoC with pins from the lower right corner to form a single MMC/SD > controller. Yes, you shouldn't do that. I may or may not work, but that > is nothing this particular SoC introduces, it's like this on many other > SoCs, perhaps even including earlier Atmel SoCs. Still, me as the guy > writing board support have to support the way the hardware is designed, > and if a board designer chooses to use pins from different iosets for a > single device I'll support it that way, no matter if a warning is shown > or not. If the users of this board are annoyed by the above warning > you'll probably receive a patch dropping it soon. Then the ioset concept > has completely vanished, but still the device trees are written like > that. > We know that mixing pins from several iosets should work for most of our devices but it won't be the case for some such as SDHCI, ISI. Yes, it is only a warning which can be easily removed. The goal of this warning was to ease support. We often have the kernel logs but no knowledge about the hardware. We could easily spot this mistake. > So, yes, my advice is to drop the ioset concept completely. If you still > insist on it then you can encode the ioset number in the pinmux define. > Only the lower 16bit are defined as the pin number, you can use the > upper 16bit like you want. You can encode the muxing in bits 16-23 and the > ioset in bits 24-31. It was exactly what I wanted to do. Every one could use the bits 31-16 to store data they need. > > > > > + if (pctldesc->complex_pin_desc && pins_prop) { > > > > + of_property_for_each_u32(np, subnode_target_type, prop, cur, val) { > > > > + pin_id = val & PINCTRL_PIN_MASK; > > > > + items_name[i++] = pctldesc->pins[pin_id].name; > > > > + } > > > > > > I don't like that even though pins have numbers here they are converted > > > to strings which the driver later has to search in a list just to > > > convert it back into the number. This is quite inefficient. > > > > > > I guess this could be optimized later, but it would be nice to have the > > > pin number directly in the driver. > > > > I know that is something you don't like but, at the moment, I need a string for > > pinctrl_utils_add_map_mux and pinctrl_utils_add_map_configs. > > Yeah, I am fine with it. This can be fixed later. I am more concerned > about the device tree bindings than about the actual implementation. The > implementation can far more easy be changed than the bindings. Using mediatek bindings is not an issue. My remaining concern is about groups, I would like to avoid 'a pin is a group'. Regards Ludovic
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c index e63ad9f..46048cb 100644 --- a/drivers/pinctrl/pinconf-generic.c +++ b/drivers/pinctrl/pinconf-generic.c @@ -278,17 +278,25 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, unsigned *reserved_maps, unsigned *num_maps, enum pinctrl_map_type type) { - int ret; + int ret, i = 0; const char *function; struct device *dev = pctldev->dev; unsigned long *configs = NULL; unsigned num_configs = 0; - unsigned reserve, strings_count; + unsigned reserve, items_count, pin_id; struct property *prop; const char *group; const char *subnode_target_type = "pins"; - - ret = of_property_count_strings(np, "pins"); + const char **items_name = NULL; + struct pinctrl_desc *pctldesc = pctldev->desc; + const __be32 *cur; + u32 val; + bool pins_prop = true; + + if (pctldesc->complex_pin_desc) + ret = of_property_count_u32_elems(np, "pins"); + else + ret = of_property_count_strings(np, "pins"); if (ret < 0) { ret = of_property_count_strings(np, "groups"); if (ret < 0) @@ -297,11 +305,12 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, if (type == PIN_MAP_TYPE_INVALID) type = PIN_MAP_TYPE_CONFIGS_GROUP; subnode_target_type = "groups"; + pins_prop = false; } else { if (type == PIN_MAP_TYPE_INVALID) type = PIN_MAP_TYPE_CONFIGS_PIN; } - strings_count = ret; + items_count = ret; ret = of_property_read_string(np, "function", &function); if (ret < 0) { @@ -326,17 +335,31 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, if (num_configs) reserve++; - reserve *= strings_count; + reserve *= items_count; ret = pinctrl_utils_reserve_map(pctldev, map, reserved_maps, num_maps, reserve); if (ret < 0) goto exit; - of_property_for_each_string(np, subnode_target_type, prop, group) { + items_name = kmalloc_array(items_count, sizeof(char *), GFP_KERNEL); + if (!items_name) + goto exit; + if (pctldesc->complex_pin_desc && pins_prop) { + of_property_for_each_u32(np, subnode_target_type, prop, cur, val) { + pin_id = val & PINCTRL_PIN_MASK; + items_name[i++] = pctldesc->pins[pin_id].name; + } + } else { + of_property_for_each_string(np, subnode_target_type, prop, group) { + items_name[i++] = group; + } + } + + for (i = 0; i < items_count; i++) { if (function) { ret = pinctrl_utils_add_map_mux(pctldev, map, - reserved_maps, num_maps, group, + reserved_maps, num_maps, items_name[i], function); if (ret < 0) goto exit; @@ -344,8 +367,8 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, if (num_configs) { ret = pinctrl_utils_add_map_configs(pctldev, map, - reserved_maps, num_maps, group, configs, - num_configs, type); + reserved_maps, num_maps, items_name[i], + configs, num_configs, type); if (ret < 0) goto exit; } @@ -353,6 +376,7 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev, ret = 0; exit: + kfree(items_name); kfree(configs); return ret; } diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h index 66e4697..116c059 100644 --- a/include/linux/pinctrl/pinctrl.h +++ b/include/linux/pinctrl/pinctrl.h @@ -45,6 +45,8 @@ struct pinctrl_pin_desc { #define PINCTRL_PIN(a, b) { .number = a, .name = b } #define PINCTRL_PIN_ANON(a) { .number = a } +#define PINCTRL_PIN_MASK 0xffff + /** * struct pinctrl_gpio_range - each pin controller can provide subranges of * the GPIO number space to be handled by the controller @@ -112,6 +114,9 @@ struct pinctrl_ops { * this pin controller * @npins: number of descriptors in the array, usually just ARRAY_SIZE() * of the pins field above + * @complex_pin_desc: some pin controllers need more information than the pin + * name. In this case, pins property uses u32 instead of string. In this + * value there is the pin number plus optional parameters. * @pctlops: pin control operation vtable, to support global concepts like * grouping of pins, this is optional. * @pmxops: pinmux operations vtable, if you support pinmuxing in your driver @@ -129,6 +134,7 @@ struct pinctrl_desc { const char *name; struct pinctrl_pin_desc const *pins; unsigned int npins; + bool complex_pin_desc; const struct pinctrl_ops *pctlops; const struct pinmux_ops *pmxops; const struct pinconf_ops *confops;
Using a string to describe a pin in the device tree can be not enough. Some controllers may need extra information to fully describe a pin. It concerns mainly controllers which have a per pin muxing approach which don't fit well the notions of groups and functions. Instead of using a pin name, a 32 bit value is used. The 16 least significant bits are used for the pin number. Other 16 bits can be used to store extra parameters. Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com> --- drivers/pinctrl/pinconf-generic.c | 44 ++++++++++++++++++++++++++++++--------- include/linux/pinctrl/pinctrl.h | 6 ++++++ 2 files changed, 40 insertions(+), 10 deletions(-)