[U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull()

[Przemyslaw Marczak]

Hello Simon,

On 02/20/2015 08:29 PM, Simon Glass wrote:
> Hi,
>
> On 20 February 2015 at 10:50, Stephen Warren <swarren at wwwdotorg.org> wrote:
>> On 02/20/2015 02:34 AM, Przemyslaw Marczak wrote:
>>>
>>> Hello,
>>>
>>> On 02/19/2015 06:09 PM, Stephen Warren wrote:
>>>>
>>>> On 02/19/2015 05:11 AM, Przemyslaw Marczak wrote:
>>>>>
>>>>> Hello,
>>>>>
>>>>> On 02/18/2015 05:39 PM, Stephen Warren wrote:
>>>>>>
>>>>>> On 02/17/2015 10:01 PM, Simon Glass wrote:
>>>>>>>
>>>>>>> +Stephen who might have an opinion on this.
>>>>>>>
>>>>>>> Hi Przemyslaw,
>>>>>>>
>>>>>>> On 17 February 2015 at 06:09, Przemyslaw Marczak
>>>>>>> <p.marczak at samsung.com> wrote:
>>>>>>>>
>>>>>>>> This commits extends:
>>>>>>>> - dm gpio ops by: 'set_pull' call
>>>>>>>> - dm gpio uclass by: dm_gpio_set_pull() function
>>>>>>>>
>>>>>>>> The pull mode is not defined so should be driver specific.
>>>>>>>
>>>>>>>
>>>>>>> It's good to implement this, but I think you should try to have a
>>>>>>> standard interface. You could define the options you want to support
>>>>>>> and pass in a standard value.
>>>>>>>
>>>>>>> Otherwise we are not really providing a driver abstraction, only an
>>>>>>> interface.
>>>>>>
>>>>>>
>>>>>> I don't think that pull is a GPIO-related function/property. At
>>>>>> least on
>>>>>> Tegra, the GPIO controller allows you to set the pin direction and the
>>>>>> output value and that's it. Configuring pull-up/down and other IO
>>>>>> related properties is done in the pinmux controller instead. I don't
>>>>>> think we want a standard API that has to touch both HW modules at once.
>>>>>> What common code needs to manipulate a GPIO's pull-up/down setting? As
>>>>>> precedent observe that pull-up/down isn't part of the Linux kernel's
>>>>>> GPIO API, but rather that's part of the SoC-specific pinctrl driver,
>>>>>> which controls pinmuxing etc.
>>>>>>
>>>>>
>>>>> This is a quite different than in the Exynos, where all the gpio
>>>>> functions and properties can be set by few registers within range of
>>>>> each gpio port base address. So in this case we don't touch another
>>>>> hardware module, we modify one of available gpio related registers.
>>>>>
>>>>> Anyway, if we want to have a single and common gpio API in the future,
>>>>> then I think it is better to add pull option.
>>>>
>>>>
>>>> Why? I'll ask again: What common driver code needs to manipulate
>>>> pull-ups?
>>>
>>>
>>> Please look at driver: drivers/gpio/s5p_gpio.c
>>>
>>> It's one driver related to one gpio hardware submodule and it takes care
>>> about standard gpio properties and also mux/pull/drv/rate.
>>>
>>> And the exynos pinmux code is only a software abstraction:
>>> arch/arm/cpu/armv7/exynos/pinmux.c
>>
>>
>> I didn't want to ask which driver implements the control of pullups, but
>> rather which other driver needs to turn pullups on/off in a standard way
>> across multiple SoCs.
>>
>> In other words, do you expect code in common/ to need to call a "set pin
>> pullup" function? If so, then we certainly need a standard API to manipulate
>> pullups. However if no common code needs to manipulate pullups, then I'd
>> argue we don't actually need a common API to do this, since there's no code
>> that would call that common API.
>
> We do currently use the GPIO to handle pullup/pulldown for some boards
> so until we have a pinmux API (which might be a long while) it seems
> reasonable for it to live there.
>
> If not, does anyone plan to write such an API?
>

Right, we uses this in most Exynos boards. But the boards uses direct 
calls to s5p gpio driver, without uclass.
I wonder if wouldn't it be better and faster to leave the board 
low-level init routines as they are now.

>>
>>
>>>>   > And the driver will
>>>>>
>>>>> implement what is required, instead of provide common and private api
>>>>> for each driver.
>>>>
>>>>
>>>> I'm not proposing driver-specific APIs, but rather having a common GPIO
>>>> API and a common pinmux API. They need to be different since different
>>>> HW modules may implement the functionality.
>>>>
>>>
>>> As in the above example, for the Exynos it's the one hw module, so it's
>>> simply.
>>>
>>>>> For the various devices it is unclear, what should be pinmux and what
>>>>> should be gpio driver.
>>>>
>>>>
>>>> How about the following are GPIO:
>>>> * Set GPIO pin direction
>>>> * Read GPIO input
>>>> * Set GPIO output value
>>>>
>>>> ... and anything else is pinmux. That's the split in Linux and AFAIK it
>>>> works out fine.
>>>>
>>>> It'd be perfectly fine for the same driver code to implement both a GPIO
>>>> and a pinmux driver, if the HW supports it.
>>>>
>>>
>>> Ok, I can drop this commit, since the current code works fine.
>>>
>>>>> Moreover in my opinion from the single external pin point of view the
>>>>> pull up/down is the property of that pin.
>>>>
>>>>
>>>> It's a property of the same pin, but semantically it's not manipulating
>>>> a GPIO-related function.
>>>>
>>>>> Actually for Exynos, the pinmux is an abstraction and uses only GPIO
>>>>> driver api in U-Boot.
>>>>>
>>>>> Do we need pinmux class?
>>>>>
>>>>> Best regards,
>>>>
>>>>
>>>>
>>>
>>> As I wrote in one of my previous e-mail, I was testing eMMC detect.
>>> And setting the pull was required for this, before call the pinmux for
>>> eMMC pins.
>>> But finally the eMMC detect seem to be not useful in case of the present
>>> 'mmc rescan' command.
>>
>>
>> Why wouldn't the pinmux driver for the whole system simply apply the board's
>> whole pinmux configuration before initializing any IO controller drivers? IO
>> controller drivers shouldn't have to initialize board-/SoC-specific pinmux,
>> but the board-/SoC-specfic code should do so.
>>
>> At most, the eMMC driver should call a function such as pinmux_emmc(), and
>> the board/SoC code should implement that as appropriate for that board. The
>> eMMC driver shouldn't have to know about applying specific pullups/downs to
>> specific pins (since those settings and pins are likely board-/SoC-specific,
>> and drivers shouldn't know about board-/SoC-specific details). The only
>
> No this way lies madness. It is how things work on Jetson and Nyan.
> Loads of opaque tables and no idea what the pins are connected to. It
> has some value for pins that U-Boot doesn't use (so we are just
> setting them up for Linux) but even then it is really opaque.
>
> We can't even sent patches to the file because it is auto-generated
> from a tool in another repo. Tiny differences between boards are
> hidden because we repeat all the information again with just a line or
> two of changes. I really don't want exynos to go that way.
>
>> exception would be if the standard IO protocol requires pullups to be
>> changed during regular operation. In which case, a specific callback from
>> the driver could be added for each protocol-mandated configuration change,
>> thus keeping the IO controller driver still completely isolated from details
>> of the pins and pinmux APIs etc.
>
> This is like the 'funcmux' in Tegra I think. I think this is more
> useful and we should use it to set up all peripheral pins. We can
> review the code, see changes, understand what they relate to, etc.
>
> Anyway this all seems off-topic from this patch.
>
> Unless someone plans to write a pinmux subsystem for U-Boot (which I
> agree would be better) I think the general approach of this patch is
> good.
>
> Regards,
> Simon
>

Ok, so there are two next versions of this patch-set.
Please decide, which one is better.

For me, at present, the current s5p_gpio api works fine for all the 
exynos based boards.
Introducing the pinmux uclass is not a quick task, now I'm trying to 
focus on pmic.

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com