From mboxrd@z Thu Jan 1 00:00:00 1970 From: swarren@wwwdotorg.org (Stephen Warren) Date: Tue, 23 Jul 2013 08:47:22 -0700 Subject: Issue with gpio and pinmux In-Reply-To: <20130722083535.GA19685@ab42.lan> References: <51E667BC.3070804@parrot.com> <51E82FBB.1080905@wwwdotorg.org> <51EB590A.4010600@wwwdotorg.org> <20130722083535.GA19685@ab42.lan> Message-ID: <51EEA58A.30206@wwwdotorg.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 07/22/2013 01:35 AM, Christian Ruppert wrote: > On Sat, Jul 20, 2013 at 09:44:10PM -0600, Stephen Warren wrote: >> On 07/20/2013 04:44 PM, Linus Walleij wrote: >>> On Thu, Jul 18, 2013 at 8:11 PM, Stephen Warren wrote: >>> >>>> The issue is that some HW muxes groups of pins at a time, so simply >>>> because that group of pins is "claimed" by a mux function, implies >>>> nothing about which specific pins in that group are actually used; some >>>> may actually still be free for usage as a GPIO. >>> >>> This we need to get into the documentation. >>> >>>> The solution here may be one of: >>>> >>>> a) Modify pinctrl /not/ to allow both a GPIO and a mux function to claim >>>> a pin *if* the group that contains the pin only contains a single pin. >>>> This is a bit of a hacky special case though; there are other situations >>>> where "dual claiming" shouldn't be allowed, and this doesn't solve those. >>>> >>>> b) Ignore the issue; if the system configuration is correct, the issue >>>> won't cause problems in practice, since nothing will be configured to >>>> "dual claim" any pin. >>>> >>>> c) Enhance pinctrl with extra information, so it knows which pins in a >>>> group are usable as GPIO even when the group is claimed by a mux >>>> function. The list of pins could theoretically vary per mux function, so >>>> you'd need a table with lookup key (pin group, mux function) and output >>>> data (list of pins that can be "dual claimed"). This would be a complete >>>> solution. If a list wasn't provided, the pinctrl core could assume that >>>> mean no dual claim was allowed. >>> >>> I guess (b) is what we have. >>> >>> But it really feels incomplete does it not? IIRC the system was strict >>> not to allow simultaneous use of pins for GPIO and other functions >>> at all until the above corner case made it necessary to relax this >>> restriction. >>> >>> A simple way to get (a) would be to just add a flag like ".strict" >>> to struct pinmux_ops but it feels cheap. >>> >>> What do we need for (c)? I think we can refactor this as the Tegra >>> and U300 should be the two platforms with such tricky groups so >>> the impact should be fairly small right now. I'm just worried it would >>> be a complex and memory-consuming thing :-/ >> >> Actually, a better way to do (c) might be to just add an extra "op" to >> struct pinmux_ops; something like validate_gpio_mux(), passing in >> information such as whether a mux function is in effect on that pin, >> whether the pin is already claimed as a GPIO, and whether the function >> is being called to check a GPIO-claim or mux-claim operation . If the op >> is missing, no "dual claim" is allowed. If the op is present, the code >> in the op validates whether the particular GPIO is allowed to be claimed >> while the particular mux function is active. >> >> That way, I think any driver that doesn't want to allow dual-claim >> doesn't have to do anything, but the few drivers that do can code up >> whatever algorithm they want; for Tegra, I'd just allow everything for >> simplicity, but we could always enhance that later if we want. > > [ An introductory note on terminology: in the following, "port" refers > to a set of pins who's mux is controlled through the same bit field in > the same register, I believe that's exactly what a "pingroup" is in pinctrl subsystem terminology. Where there is already an existing name for something, it'd be best to use that so that every person doesn't use a different name for everything. > "interface" refers to the set of pins comprising a > functional unit, e.g. SCL and SDA of an I2C interface or the four pins > of an SPI interface ] > > In my understanding, the problem stems from the fact that we always > claim an entire port instead of just the pins required for a given > functionality. Yes. > The pinctrl core is already well equipped to manage > claiming interfaces instead of ports and thus a fourth alternative would > be to I would argue that's not the case. pinctrl knows absolutely nothing about "interfaces"; there is no data structure defines that describes interfaces at all. That's not saying such a thing could not be added; I'm simply arguing that "well equipped" doesn't seem to be true at the moment. > d) implement gpio/pin conflict management in the already existing > callbacks inside the driver: Today, both enable and gpio_request_enable > exist (and can refuse a request for a configuration/pin). > If we only claim the pins used by a given interface instead of the > entire port, conflict management becomes natural (see > http://lkml.indiana.edu/hypermail/linux/kernel/1306.2/00749.html and the > following). OK, that's basically the same as my proposal to add a new callback; it's simply re-using and existing driver callback rather than creating a new one solely for conflict checking. Re-using the existing callback(s) seems fine, with appropriate prototype modification to pass back the list of pins that can-or-can't be dual-claimed I suppose. > IMHO, the pinctrl drivers are the right place for everything but the > most basic pin conflict management since constraints on what can be > muxed at the same time are highly hardware implementation dependent. One > could e.g. imagine an implementation where a GPIO can override and snoop > a pin's output value in one mux configuration, just snoop the pin value > in another and do neither in a third. > > I see two (independent) ways to enhance the pinctrl core in order to > support drivers in this task: > a) Add two functions pin_is_claimed and gpio_is_claimed which allow the > pinctrl driver to query if a given pin/gpio is available for muxing. > b) Add four functions lock_gpio/unlock_gpio and lock_pin/unlock_pin > which allow the pinctrl driver to prevent the core from granting > requests on GPIOs/pins. Yes, either of those are basically what I proposed in my followup to the email you're replying to. > This solution also has the advantage that it doesn't modify the default > behaviour of the core and it thus won't break existing drivers.