From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Tue, 24 Feb 2015 10:44:07 +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> <54EB064E.7070800@samsung.com> <54EB5BCB.7010109@samsung.com> Message-ID: <54EC47E7.7080300@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, On 02/23/2015 06:50 PM, Simon Glass wrote: > 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 > 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