[U-Boot] [RFC v3 PATCH 2/4] pinctrl: add pin control uclass support
Masahiro Yamada
yamada.masahiro at socionext.com
Tue Aug 25 08:32:32 CEST 2015
Simon,
I've just posted v4.
Sorry for the delay.
2015-08-12 23:16 GMT+09:00 Simon Glass <sjg at chromium.org>:
> Hi Masahiro,
>
> On 10 August 2015 at 10:05, Masahiro Yamada
> <yamada.masahiro at socionext.com> wrote:
>> This creates a new framework for handling of pin control devices,
>> i.e. devices that control different aspects of package pins.
>>
>> This uclass handles pinmuxing and pin configuration; pinmuxing
>> controls switching among silicon blocks that share certain physical
>> pins, pin configuration handles electronic properties such as pin-
>> biasing, load capacitance etc.
>>
>> This framework supports the same device tree bindings, but if you
>> do not need full interface support, you can disable some features
>> to reduce memory foot print.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com>
>> ---
>>
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/core/device.c | 4 +
>> drivers/pinctrl/Kconfig | 42 +++++
>> drivers/pinctrl/Makefile | 2 +
>> drivers/pinctrl/pinctrl-generic.c | 351 ++++++++++++++++++++++++++++++++++++++
>> drivers/pinctrl/pinctrl-uclass.c | 151 ++++++++++++++++
>> include/dm/pinctrl.h | 218 +++++++++++++++++++++++
>> include/dm/uclass-id.h | 2 +
>> 9 files changed, 773 insertions(+)
>> create mode 100644 drivers/pinctrl/Kconfig
>> create mode 100644 drivers/pinctrl/Makefile
>> create mode 100644 drivers/pinctrl/pinctrl-generic.c
>> create mode 100644 drivers/pinctrl/pinctrl-uclass.c
>> create mode 100644 include/dm/pinctrl.h
>>
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index 092bc02..2b9933f 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -32,6 +32,8 @@ source "drivers/i2c/Kconfig"
>>
>> source "drivers/spi/Kconfig"
>>
>> +source "drivers/pinctrl/Kconfig"
>> +
>> source "drivers/gpio/Kconfig"
>>
>> source "drivers/power/Kconfig"
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index a721ec8..9d0a595 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -1,6 +1,7 @@
>> obj-$(CONFIG_$(SPL_)DM) += core/
>> obj-$(CONFIG_$(SPL_)CLK) += clk/
>> obj-$(CONFIG_$(SPL_)LED) += led/
>> +obj-$(CONFIG_$(SPL_)PINCTRL) += pinctrl/
>> obj-$(CONFIG_$(SPL_)RAM) += ram/
>>
>> ifdef CONFIG_SPL_BUILD
>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>> index 634070c..767b7fe 100644
>> --- a/drivers/core/device.c
>> +++ b/drivers/core/device.c
>> @@ -15,6 +15,7 @@
>> #include <dm/device.h>
>> #include <dm/device-internal.h>
>> #include <dm/lists.h>
>> +#include <dm/pinctrl.h>
>> #include <dm/platdata.h>
>> #include <dm/uclass.h>
>> #include <dm/uclass-internal.h>
>> @@ -277,6 +278,9 @@ int device_probe_child(struct udevice *dev, void *parent_priv)
>>
>> dev->flags |= DM_FLAG_ACTIVATED;
>>
>> + /* continue regardless of the result of pinctrl */
>> + pinctrl_select_state(dev, "default");
>> +
>> ret = uclass_pre_probe_device(dev);
>> if (ret)
>> goto fail;
>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>> new file mode 100644
>> index 0000000..3f4f4b3
>> --- /dev/null
>> +++ b/drivers/pinctrl/Kconfig
>> @@ -0,0 +1,42 @@
>> +#
>> +# PINCTRL infrastructure and drivers
>> +#
>> +
>> +menu "Pin controllers"
>> +
>> +config PINCTRL
>> + bool "Support pin controllers"
>> +
>> +config PINCTRL_GENERIC
>> + bool "Support generic pin controllers"
>> + depends on PINCTRL
>
> Can you add some help for these - explaining what each one means and
> why to enable it.
Done.
Feel free to add more detailed explanations you think is necessary.
http://patchwork.ozlabs.org/patch/510080/
>> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
>> new file mode 100644
>> index 0000000..a713c7d
>> --- /dev/null
>> +++ b/drivers/pinctrl/Makefile
>
> Does this need a license header?
I do not think it is important for Makefiles.
>> +
>> +/**
>> + * pinctrl_pin_name_to_selector() - return the pin selector for a pin
>> + * @dev: pin controller device
>> + * @pin: the pin name to look up
>
> @return, please fix globally.
Done.
>> + */
>> +static int pinctrl_pin_name_to_selector(struct udevice *dev, const char *pin)
>> +{
>> + const struct pinctrl_ops *ops = dev->driver->ops;
>
> Please create a #define for this as with spi.h for example.
Done.
>> + unsigned npins, selector = 0;
>> +
>> + if (!ops->get_pins_count || !ops->get_pin_name) {
>> + dev_err(dev, "get_pins_count or get_pin_name missing\n");
>
> Should this be debug() or dev_warn()? It would be nice to compile
> these out unless debugging.
I chose dev_dbg().
>> + return -EINVAL;
>
> That is normally used for an invalid device tree arg. How about -ENOSYS?
This is the comment block in U-Boot:
#define ENOSYS 38 /* Function not implemented */
And this is the one in Linux:
/*
* This error code is special: arch syscall entry code will return
* -ENOSYS if users try to call a syscall that doesn't exist. To keep
* failures of syscalls that really do exist distinguishable from
* failures due to attempts to use a nonexistent syscall, syscall
* implementations should refrain from returning -ENOSYS.
*/
#define ENOSYS 38 /* Invalid system call number */
>From the comment above, I hesitate to use -ENOSYS for normal error cases.
I chose ENOTSUPP for unimplemented functions and it is widely used
in Linux's pinctrl drivers.
>> + }
>> +
>> + npins = ops->get_pins_count(dev);
>> +
>> + /* See if this pctldev has this pin */
>> + while (selector < npins) {
>
> How about:
>
> for (selector = 0; selector < npins; selected++)
Good idea. Done.
>> + const char *pname = ops->get_pin_name(dev, selector);
>> +
>> + if (!strcmp(pin, pname))
>> + return selector;
>> +
>> + selector++;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +/**
>> + * pinctrl_group_name_to_selector() - return the group selector for a group
>> + * @dev: pin controller device
>> + * @group: the pin group name to look up
>
> @return
Done.
>> + */
>> +static int pinctrl_group_name_to_selector(struct udevice *dev,
>> + const char *group)
>> +{
>> + const struct pinctrl_ops *ops = dev->driver->ops;
>> + unsigned ngroups, selector = 0;
>> +
>> + if (!ops->get_groups_count || !ops->get_group_name) {
>> + dev_err(dev, "get_groups_count or get_group_name missing\n");
>> + return -EINVAL;
>> + }
>> +
>> + ngroups = ops->get_groups_count(dev);
>> +
>> + /* See if this pctldev has this group */
>> + while (selector < ngroups) {
>> + const char *gname = ops->get_group_name(dev, selector);
>> +
>> + if (!strcmp(group, gname))
>> + return selector;
>> +
>> + selector++;
>> + }
>> +
>> + return -EINVAL;
>
> I think this means that the device tree is missing something, in which
> case this is fine. If not you might consider -ENOENT.
Actually, the pinctrl driver subsystem returns -EINVAL in this case
(see linux/drivers/pinctrl/pinmux.c), but looks like you do not prefer
-EINVAL here.
I replaced it with -ENOTSUPP.
>> + for (prop_offset = fdt_first_property_offset(fdt, node_offset);
>> + prop_offset > 0;
>> + prop_offset = fdt_next_property_offset(fdt, prop_offset)) {
>> + value = fdt_getprop_by_offset(fdt, prop_offset,
>> + &propname, &len);
>> + if (!value)
>> + return -EINVAL;
>> +
>> + if (!strcmp(propname, "function")) {
>> + func_selector = pinmux_func_name_to_selector(dev,
>> + value);
>> + if (func_selector < 0)
>> + return func_selector;
>> + ret = pinmux_enable_setting(dev, is_group,
>> + selector,
>> + func_selector);
>> + } else {
>> + param = pinconf_prop_name_to_param(dev, propname,
>> + &default_val);
>> + if (param < 0)
>> + continue; /* just skip unknown properties */
>> +
>> + if (len > 0)
>
> Strictly speaking, len > sizeof(fdt32_t)
Do you mean len >= sizeof(fdt32_t) ?
Assuming so, fixed in v4.
>> +int pinctrl_generic_set_state(struct udevice *dev, struct udevice *config)
>> +{
>> + struct udevice *child;
>> + int ret;
>> +
>> + ret = pinctrl_generic_set_state_subnode(dev, config);
>> + if (ret)
>> + return ret;
>> +
>> + list_for_each_entry(child, &config->child_head, sibling_node) {
>> + ret = pinctrl_generic_set_state_subnode(dev, child);
>> + if (ret)
>> + return ret;
>
> I try to keep access to internal lists within driver/core. Consider
> using device_find_first/next_child instead. Or perhaps create an
> iterator device_foreach_child().
Done.
(but I still think my original code is simpler. Maybe a new iterator
is worth creating)
>> +static int pinctrl_config_one(struct udevice *config)
>> +{
>> + struct udevice *pctldev;
>> + const struct pinctrl_ops *ops;
>> +
>> + pctldev = config;
>> + for (;;) {
>> + pctldev = dev_get_parent(pctldev);
>> + if (!pctldev) {
>> + dev_err(config, "could not find pctldev\n");
>> + return -EINVAL;
>> + }
>> + if (pctldev->uclass->uc_drv->id == UCLASS_PINCTRL)
>> + break;
>> + }
>> +
>> + ops = pctldev->driver->ops;
>
> Use a #define for this as other uclasses do.
>
> assert(ops), or check for NULL and return -ENOSYS.
I chose "check the valid pointer or -ENOTSUPP"
>> + return ops->set_state(pctldev, config);
>> +}
>> +
>> +int pinctrl_select_state(struct udevice *dev, const char *statename)
>> +{
>> + DECLARE_GLOBAL_DATA_PTR;
>
> Can we put that at the top of the file?
Done.
>> +/**
>> + * pinconfig_post-bind() - post binding for PINCONFIG uclass
>> + * Recursively bind its children as pinconfig devices.
>
> What is the use case for recursive binding?
It is allowed to have grouping nodes.
The following is the real use-case (linux/arch/arm/boot/dts/zynq-zc706.dts):
The nodes "gem0-default", "gpio0-default", "i2c0-default" are just grouping
their children. The pin-mux, pin-conf settings are described in the
lower level nodes.
In this case, binding should be done recursively.
&pinctrl0 {
pinctrl_gem0_default: gem0-default {
mux {
function = "ethernet0";
groups = "ethernet0_0_grp";
};
conf {
groups = "ethernet0_0_grp";
slew-rate = <0>;
io-standard = <4>;
};
conf-rx {
pins = "MIO22", "MIO23", "MIO24", "MIO25",
"MIO26", "MIO27";
bias-high-impedance;
low-power-disable;
};
conf-tx {
pins = "MIO16", "MIO17", "MIO18", "MIO19",
"MIO20", "MIO21";
low-power-enable;
bias-disable;
};
mux-mdio {
function = "mdio0";
groups = "mdio0_0_grp";
};
conf-mdio {
groups = "mdio0_0_grp";
slew-rate = <0>;
io-standard = <1>;
bias-disable;
};
};
pinctrl_gpio0_default: gpio0-default {
mux {
function = "gpio0";
groups = "gpio0_7_grp", "gpio0_46_grp", "gpio0_47_grp";
};
conf {
groups = "gpio0_7_grp", "gpio0_46_grp", "gpio0_47_grp";
slew-rate = <0>;
io-standard = <1>;
};
conf-pull-up {
pins = "MIO46", "MIO47";
bias-pull-up;
};
conf-pull-none {
pins = "MIO7";
bias-disable;
};
};
pinctrl_i2c0_default: i2c0-default {
mux {
groups = "i2c0_10_grp";
function = "i2c0";
};
conf {
groups = "i2c0_10_grp";
bias-pull-up;
slew-rate = <0>;
io-standard = <1>;
};
};
pinctrl_sdhci0_default: sdhci0-default {
mux {
groups = "sdio0_2_grp";
function = "sdio0";
};
conf {
groups = "sdio0_2_grp";
slew-rate = <0>;
io-standard = <1>;
bias-disable;
};
mux-cd {
groups = "gpio0_14_grp";
function = "sdio0_cd";
};
conf-cd {
groups = "gpio0_14_grp";
bias-high-impedance;
bias-pull-up;
slew-rate = <0>;
io-standard = <1>;
};
mux-wp {
groups = "gpio0_15_grp";
function = "sdio0_wp";
};
conf-wp {
groups = "gpio0_15_grp";
bias-high-impedance;
bias-pull-up;
slew-rate = <0>;
io-standard = <1>;
};
};
>> +#define PIN_CONFIG_BIAS_DISABLE 0
>> +#define PIN_CONFIG_BIAS_HIGH_IMPEDANCE 1
>> +#define PIN_CONFIG_BIAS_BUS_HOLD 2
>> +#define PIN_CONFIG_BIAS_PULL_UP 3
>> +#define PIN_CONFIG_BIAS_PULL_DOWN 4
>> +#define PIN_CONFIG_BIAS_PULL_PIN_DEFAULT 5
>> +#define PIN_CONFIG_DRIVE_PUSH_PULL 6
>> +#define PIN_CONFIG_DRIVE_OPEN_DRAIN 7
>> +#define PIN_CONFIG_DRIVE_OPEN_SOURCE 8
>> +#define PIN_CONFIG_DRIVE_STRENGTH 9
>> +#define PIN_CONFIG_INPUT_ENABLE 10
>> +#define PIN_CONFIG_INPUT_SCHMITT_ENABLE 11
>> +#define PIN_CONFIG_INPUT_SCHMITT 12
>> +#define PIN_CONFIG_INPUT_DEBOUNCE 13
>> +#define PIN_CONFIG_POWER_SOURCE 14
>> +#define PIN_CONFIG_SLEW_RATE 15
>> +#define PIN_CONFIG_LOW_POWER_MODE 16
>> +#define PIN_CONFIG_OUTPUT 17
>
> Use enum?
Low-level drivers are allowed to have vendor-specific parameters
in addition to these generic parameters.
If we defined enum only for these 18 parameters,
it would be difficult to do so.
>> +#define PIN_CONFIG_END 0x7FFF
>
>> +
>> +#if CONFIG_IS_ENABLED(PINCTRL)
>> +/**
>> + * pinctrl_select_state() - set a device to a given state
>> + *
>> + * @dev: peripheral device
>> + * @statename: state name, like "default"
>> + */
>> +int pinctrl_select_state(struct udevice *dev, const char *statename);
>> +#else
>> +static inline int pinctrl_select_state(struct udevice *dev,
>> + const char *statename)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> +#if CONFIG_IS_ENABLED(PINCTRL_GENERIC)
>
> Do you think PINCTRL_SIMPLE might be a better name?
They have different meanings.
PINCTRL_SIMPLE - switch between full-pinctrl and simple-pinctrl
simple-pinctrl does not require DTS at all
PINCTRL_GENERIC - enable generic DTS interface
This depends on full-pinctrl.
This feature is useful to handle generic properties
such as "pins", "groups", "functions" in DTS.
>> +
>> +#endif /* __PINCTRL_H */
>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>> index c744044..a2fb6de 100644
>> --- a/include/dm/uclass-id.h
>> +++ b/include/dm/uclass-id.h
>> @@ -44,6 +44,8 @@ enum uclass_id {
>> UCLASS_PCH, /* x86 platform controller hub */
>> UCLASS_PCI, /* PCI bus */
>> UCLASS_PCI_GENERIC, /* Generic PCI bus device */
>> + UCLASS_PINCTRL, /* Pinctrl device */
>
> Expand - e.g. Pin control and multiplexing device
Done.
>> + UCLASS_PINCONFIG, /* Pinconfig node device */
>
> Pin configuration
Done.
--
Best Regards
Masahiro Yamada
More information about the U-Boot
mailing list