From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Mon, 23 Feb 2015 10:50:22 -0700 Subject: [U-Boot] [PATCH 1/4] dm: gpio: extend gpio api by dm_gpio_set_pull() In-Reply-To: <54EB5BCB.7010109@samsung.com> 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> <54EB064E.7070800@samsung.com> <54EB5BCB.7010109@samsung.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Przemyslaw, On 23 February 2015 at 09:56, Przemyslaw Marczak wrote: > Hello, > > > On 02/23/2015 04:30 PM, Simon Glass wrote: >> >> Hi, >> >> On 23 February 2015 at 03:51, Przemyslaw Marczak >> wrote: >>> >>> >>> 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. >> >> >> 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