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

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello Stephen,

On 02/20/2015 06:50 PM, Stephen Warren 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@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.
>

Probably none.

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

Ok, you are right.

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

This is clean, and the Exynos pinmux code is implemented in this way.

>
> 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 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 also clean, and the mmc driver for exynos, calls the pinmux with 
the device id and flag as an arguments. This works fine.

The exception which I need, was to set just one emmc pin as input with 
proper pull-up (which was defined in emmc dts node) and check the value.
This was temporary, before the proper pinmux call for emmc 
configuration, after card detect.

Of course this was a hack, and for each board, the pull value was 
defined in dts(tested on 2 boards). And this was my tests only, I didn't 
send this code to list, but now I see that probably better could be some 
board specific function like board_emmc_detected().

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