From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Mon, 23 Feb 2015 11:21:53 +0100 Subject: [U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull() In-Reply-To: <54E773C8.3090601@wwwdotorg.org> References: <1424178544-28632-1-git-send-email-p.marczak@samsung.com> <1424178544-28632-2-git-send-email-p.marczak@samsung.com> <54E4C02A.3030905@wwwdotorg.org> <54E5D2DC.1080508@samsung.com> <54E618CA.9000600@wwwdotorg.org> <54E6FFC2.1020508@samsung.com> <54E773C8.3090601@wwwdotorg.org> Message-ID: <54EAFF41.7060601@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>>>>> 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