[U-Boot] [PATCH v4 03/33] pinctrl: Add the concept of peripheral IDs
Masahiro Yamada
yamada.masahiro at socionext.com
Wed Aug 26 06:51:05 CEST 2015
2015-08-26 13:38 GMT+09:00 Simon Glass <sjg at chromium.org>:
> Hi Masahiro,
>
> On 25 August 2015 at 21:30, Masahiro Yamada
> <yamada.masahiro at socionext.com> wrote:
>> Hi Simon,
>>
>>
>>
>> 2015-08-25 0:12 GMT+09:00 Simon Glass <sjg at chromium.org>:
>>> My original pinctrl patch operating using a peripheral ID enum. This was
>>> shared between pinmux and clock and provides an easy way to specify a device
>>> that needs to be controlled, even it is does not (yet) have a driver within
>>> driver model.
>>>
>>> Masahiro's new simple pinctrl gets around this by providing a
>>> set_state_simple() pinctrl method. By passing a device to that call the
>>> peripheral ID becomes unnecessary. If the driver needs it, it can calculate
>>> it itself and use it internally.
>>>
>>> However this does not solve the problem for peripheral clocks. The 'pure'
>>> solution would be to pass a driver to the clock uclass also. But this
>>> requires that all devices should have a driver, and a struct udevide. Also
>>> a key optimisation of the clock uclass is allowing a peripheral clock to
>>> be set even when there is no device for that clock.
>>>
>>> There may be a better way to achive the same goal, but for now it seems
>>> expedient to add in peripheral ID to the pinctrl uclass. Two methods are
>>> added - one to get the peripheral ID and one to select it. The existing
>>> set_state_simple() is effectively the union of these.
>>>
>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>> ---
>>>
>>> Changes in v4: None
>>> Changes in v3: None
>>> Changes in v2: None
>>>
>>> drivers/pinctrl/pinctrl-uclass.c | 33 ++++++++++++++++++++++
>>> include/dm/pinctrl.h | 61 ++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 94 insertions(+)
>>>
>>> diff --git a/drivers/pinctrl/pinctrl-uclass.c b/drivers/pinctrl/pinctrl-uclass.c
>>> index 01b0cf8..1d90fb0 100644
>>> --- a/drivers/pinctrl/pinctrl-uclass.c
>>> +++ b/drivers/pinctrl/pinctrl-uclass.c
>>> @@ -9,6 +9,7 @@
>>> #include <dm/device.h>
>>> #include <dm/lists.h>
>>> #include <dm/pinctrl.h>
>>> +#include <dm/root.h>
>>> #include <dm/uclass.h>
>>> #include <linux/err.h>
>>> #include <linux/list.h>
>>> @@ -36,9 +37,41 @@ int pinctrl_select_state(struct udevice *dev, const char *ignored)
>>> return ops->set_state_simple(pctldev, dev);
>>> }
>>>
>>> +int pinctrl_request(struct udevice *dev, int func, int flags)
>>> +{
>>> + struct pinctrl_ops *ops = pinctrl_get_ops(dev);
>>> +
>>> + if (!ops->request)
>>> + return -ENOSYS;
>>> +
>>> + return ops->request(dev, func, flags);
>>> +}
>>> +
>>> +int pinctrl_request_noflags(struct udevice *dev, int func)
>>> +{
>>> + return pinctrl_request(dev, func, 0);
>>> +}
>>> +
>>> +int pinctrl_get_periph_id(struct udevice *dev, struct udevice *periph)
>>> +{
>>> + struct pinctrl_ops *ops = pinctrl_get_ops(dev);
>>> +
>>> + if (!ops->get_periph_id)
>>> + return -ENOSYS;
>>> +
>>> + return ops->get_periph_id(dev, periph);
>>> +}
>>
>>
>> If _get_periph_id() is shared between clk and pinctrl (that what I understood
>> from your statement), where is it defined? Under arch/arm/mach-<soc>/ ?
>
> Yes.
>
>>
>>
>>
>>
>>> +static int pinctrl_post_bind(struct udevice *dev)
>>> +{
>>> + /* Scan the bus for devices */
>>> + return dm_scan_fdt_node(dev, gd->fdt_blob, dev->of_offset, false);
>>> +}
>>
>>
>> I do not think this work.
>>
>> dm_scan_fdt_node() requires child nodes to be compatible to something
>> for binding.
>>
>> Pin configuration nodes do not have '.compatible' properties.
>
> True in many cases, in which case it does not harm, but Rockchip has them:
>
> http://git.denx.de/?p=u-boot-dm.git;a=blob;f=arch/arm/dts/rk3288.dtsi;h=0f497099679470ea39078d6ca9e5b1ae550995b0;hb=refs/heads/rockchip-working
My bad.
It is possible a pinctrl device can have other devices under it (in
this case, GPIO).
So, it turned out my patch should be fixed.
device_bind_driver_to_node() should be called for devices that have no
.compatible.
>>
>>
>>
>>
>>> UCLASS_DRIVER(pinctrl) = {
>>> .id = UCLASS_PINCTRL,
>>> .name = "pinctrl",
>>> + .post_bind = pinctrl_post_bind,
>>> };
>>>
>>> #else
>>> diff --git a/include/dm/pinctrl.h b/include/dm/pinctrl.h
>>> index b2ba0b4..2f9bf67 100644
>>> --- a/include/dm/pinctrl.h
>>> +++ b/include/dm/pinctrl.h
>>> @@ -85,10 +85,37 @@ struct pinctrl_ops {
>>> int (*pinconf_group_set)(struct udevice *dev, unsigned group_selector,
>>> unsigned param, unsigned argument);
>>> int (*set_state)(struct udevice *dev, struct udevice *config);
>>> +
>>> /* for pinctrl-simple */
>>> int (*set_state_simple)(struct udevice *dev, struct udevice *periph);
>>> + /**
>>> + * request() - Request a particular pinctrl function
>>> + *
>>> + * This activates the selected function.
>>> + *
>>> + * @dev: Device to adjust (UCLASS_PINCTRL)
>>> + * @func: Function number (driver-specific)
>>> + * @return 0 if OK, -ve on error
>>> + */
>>> + int (*request)(struct udevice *dev, int func, int flags);
>>> +
>>> + /**
>>> + * get_periph_id() - get the peripheral ID for a device
>>> + *
>>> + * This generally looks at the peripheral's device tree node to work
>>> + * out the peripheral ID. The return value is normally interpreted as
>>> + * enum periph_id. so long as this is defined by the platform (which it
>>> + * should be).
>>> + *
>>> + * @dev: Pinctrl device to use for decoding
>>> + * @periph: Device to check
>>> + * @return peripheral ID of @periph, or -ENOENT on error
>>> + */
>>> + int (*get_periph_id)(struct udevice *dev, struct udevice *periph);
>>> };
>>>
>>> +#define pinctrl_get_ops(dev) ((struct pinctrl_ops *)(dev)->driver->ops)
>>> +
>>> /**
>>> * Generic pin configuration paramters
>>> *
>>> @@ -217,4 +244,38 @@ static inline int pinctrl_generic_set_state(struct udevice *pctldev,
>>> }
>>> #endif
>>>
>>> +/**
>>> + * pinctrl_request() - Request a particular pinctrl function
>>> + *
>>> + * @dev: Device to check (UCLASS_PINCTRL)
>>> + * @func: Function number (driver-specific)
>>> + * @flags: Flags (driver-specific)
>>> + * @return 0 if OK, -ve on error
>>> + */
>>> +int pinctrl_request(struct udevice *dev, int func, int flags);
>>> +
>>> +/**
>>> + * pinctrl_request_noflags() - Request a particular pinctrl function
>>> + *
>>> + * This is similar to pinctrl_request() but uses 0 for @flags.
>>> + *
>>> + * @dev: Device to check (UCLASS_PINCTRL)
>>> + * @func: Function number (driver-specific)
>>> + * @return 0 if OK, -ve on error
>>> + */
>>> +int pinctrl_request_noflags(struct udevice *dev, int func);
>>> +
>>> +/**
>>> + * pinctrl_get_periph_id() - get the peripheral ID for a device
>>> + *
>>> + * This generally looks at the peripheral's device tree node to work out the
>>> + * peripheral ID. The return value is normally interpreted as enum periph_id.
>>> + * so long as this is defined by the platform (which it should be).
>>> + *
>>> + * @dev: Pinctrl device to use for decoding
>>> + * @periph: Device to check
>>> + * @return peripheral ID of @periph, or -ENOENT on error
>>> + */
>>> +int pinctrl_get_periph_id(struct udevice *dev, struct udevice *periph);
>>> +
>>> #endif /* __PINCTRL_H */
>>
>>
>>
>> I am guessing these functions are only invoked from .set_state_simple(),
>> If so, should they be exported to struct pinctrl_ops?
>>
>> I expect callbacks are invoked from the core framework (uclass).
>>
>
> They can be involved from other places. In particular you need to call
> get_periph_id() to get the peripheral ID for use with clocks.
>
In the first place, pinctrl_get_periph_id() should not be defined in
a pinctrl driver.
The first argument, pinctrl device, is not needed to calculate the
peripheral ID,
and the actual definition is placed under mach-<soc>, right?
--
Best Regards
Masahiro Yamada
More information about the U-Boot
mailing list