From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Mon, 23 Feb 2015 11:51:58 +0100 Subject: [U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull() In-Reply-To: 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: <54EB064E.7070800@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 Simon, On 02/20/2015 08:29 PM, Simon Glass wrote: > Hi, > > On 20 February 2015 at 10:50, 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. >> >> 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