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

Tue Feb 24 10:44:07 CET 2015

Hello,

On 02/23/2015 06:50 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 23 February 2015 at 09:56, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
>> Hello,
>>
>>
>> On 02/23/2015 04:30 PM, Simon Glass wrote:
>>>
>>> Hi,
>>>
>>> On 23 February 2015 at 03:51, Przemyslaw Marczak <p.marczak at samsung.com>
>>> wrote:
>>>>
>>>>
>>>> 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.
>>>
>>>
>>> OK, then I think we should probably leave it as it is. If we add
>>> pull-ups to driver model it should be done with pinctl as Stephen
>>> says. I doubt this is a huge task, since we can likely port over the
>>> code from Linux. But for now I think we should keep with the s5p API
>>> until someone takes on pinctl.
>>>
>>> Regards,
>>> Simon
>>>
>>
>> Great, in this case, can the v3 be accepted?
>
> v3 of what please? Can you give me a pointer?
>
> Regards,
> Simon
>

oops, sorry, I didn't change the cc list...
There was, v2 and v3 on the list. But the v3 is included in mmc tree 
pull request:

http://patchwork.ozlabs.org/patch/442639/

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