Hi Masahiro,
On 26 August 2015 at 22:39, Masahiro Yamada
<yamada.masah...@socionext.com> wrote:
> 2015-08-26 22:20 GMT+09:00 Simon Glass <s...@chromium.org>:
>> Hi Masahiro,
>>
>> On 25 August 2015 at 21:51, Masahiro Yamada
>> <yamada.masah...@socionext.com> wrote:
>>> 2015-08-26 13:38 GMT+09:00 Simon Glass <s...@chromium.org>:
>>>> Hi Masahiro,
>>>>
>>>> On 25 August 2015 at 21:30, Masahiro Yamada
>>>> <yamada.masah...@socionext.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>>>
>>>>>
>>>>> 2015-08-25 0:12 GMT+09:00 Simon Glass <s...@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 <s...@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.
>>>
>>
>> OK.
>
>
> So, dm_scan_fdt_node() should be moved to rk3288_pinctrl_bind(), I think.
>
> Having GPIO nodes under the pinctrl node is a rockchip-specific matter.
Does it hurt to try to make it generic? Are you saying that is it wrong?
>>>>>> 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?
>>
>> So are you suggesting that each SoC provides a function (not in the
>> driver model framework) to get the peripheral ID in the simple pinctrl
>> case? This might be similar to clock_decode_periph_id(const void
>> *blob, int node) in tegra.
>
>
> Only for those SoCs that use pinctrl-simple and clk-simple.
>
> Given that the peripheral ID is driver-wide concept that is shared
> between pinctrl-simple and clk-simple,
> the ID decoder should be placed in common location, so that
> pinctrl-simple should not depends on clk-simple, and vice versa.
>
Yes I agree.
However I have not got to this yet and time is marching on.
In order to support generic platforms we would need to be able to
supply a conversion function for any SoC. I think the whole peripheral
ID thing needs a bit of a closer look so I'd like to do that
separately. I suspect there might be another way to achieve this.
>
>
>
>> In fact if we implement a full clock controller binding (similar to
>> how you have implemented pinctrl) then we can avoid the need for
>> peripheral IDs once driver model is fully up. The need then exists
>> only in SPL, due to code size/memory constraints. It may perhaps also
>> be needed before relocation in U-Boot proper due to execution time
>> constraints but we can limit the number of devices brought up using
>> u-boot,dm-pre-reloc.
>>
>> It would be good to limit the spread of the peripheral ID and only use
>> it when needed.
>>
>> I'll take a look at creating a function outside of pinctrl which
>> returns a peripheral ID. That deals with the get_periph_id() method in
>> pinctrl.
>>
>> However this still leaves the request() method - which takes a
>> peripheral ID. This is needed I think because:the device being set up
>> might not have a udevice in SPL. What do you think?
>>
>> Regards,
>> Simon
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot@lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>
>
>
> --
> Best Regards
> Masahiro Yamada
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
