[U-Boot] [PATCH v4 03/33] pinctrl: Add the concept of peripheral IDs

[Simon Glass]

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

>
>
>
>
>
>>  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.

Regards,
Simon