| Message ID | 1384881351-10782-1-git-send-email-t.figa@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/19/2013 10:15 AM, Tomasz Figa wrote: > This patch extends the range of settings configurable via pinfunc API > to cover pin value as well. This allows configuration of default values > of pins. Shouldn't there be a driver that acquires the GPIO that's output to the pin, and configures the output value? IIRC there have been previous discussions re: having a list of e.g. initial GPIO output values in DT, and that was rejected, and this patch seems to be doing almost the exact same thing, just at the pinctrl level rather than GPIO level. That all said, I admit this could be a useful feature...
On Tue, Nov 19, 2013 at 10:46 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 11/19/2013 10:15 AM, Tomasz Figa wrote: >> This patch extends the range of settings configurable via pinfunc API >> to cover pin value as well. This allows configuration of default values >> of pins. > > Shouldn't there be a driver that acquires the GPIO that's output to the > pin, and configures the output value? IIRC there have been previous > discussions re: having a list of e.g. initial GPIO output values in DT, > and that was rejected, and this patch seems to be doing almost the exact > same thing, just at the pinctrl level rather than GPIO level. > > That all said, I admit this could be a useful feature... I haven't followed all of the previous discussions, but I know I've run into scenarios where something like this would be useful. The one that comes to mind is: * We've got GPIOs that default at bootup to a pulled up input since the default state of the pin should be "high". * These pins are really intended to be outputs, like an "enable", "reset", or "power down" line for a peripheral. The pullup is strong enough to give us a good default state but we really want outputs. * We'd like to provide this GPIO to a peripheral through device tree. ...and we'd like all the pinmux to be setup automatically so we use pinctrl-names = "default". * If we set the pinmux up as "output" then there's a chance that the line will glitch at bootup since the pinmux happens (changing the pin to output) before the driver has a chance to run. Does that sound like the same scenario you're trying to solve Tomasz?
On 11/19/2013 11:59 AM, Doug Anderson wrote: > On Tue, Nov 19, 2013 at 10:46 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 11/19/2013 10:15 AM, Tomasz Figa wrote: >>> This patch extends the range of settings configurable via pinfunc API >>> to cover pin value as well. This allows configuration of default values >>> of pins. >> >> Shouldn't there be a driver that acquires the GPIO that's output to the >> pin, and configures the output value? IIRC there have been previous >> discussions re: having a list of e.g. initial GPIO output values in DT, >> and that was rejected, and this patch seems to be doing almost the exact >> same thing, just at the pinctrl level rather than GPIO level. >> >> That all said, I admit this could be a useful feature... > > I haven't followed all of the previous discussions, but I know I've > run into scenarios where something like this would be useful. The one > that comes to mind is: > > * We've got GPIOs that default at bootup to a pulled up input since > the default state of the pin should be "high". > > * These pins are really intended to be outputs, like an "enable", > "reset", or "power down" line for a peripheral. The pullup is strong > enough to give us a good default state but we really want outputs. > > * We'd like to provide this GPIO to a peripheral through device tree. > ...and we'd like all the pinmux to be setup automatically so we use > pinctrl-names = "default". > > * If we set the pinmux up as "output" then there's a chance that the > line will glitch at bootup since the pinmux happens (changing the pin > to output) before the driver has a chance to run. I think that last point should be addressed by having a driver that owns the GPIO set it to the desired output level, and the implementation of the SoC's GPIO driver communicate with the pinctrl driver (which might be the same driver; not sure here) so that gpio_direction_output() causes the pinctrl HW to be programmed as output only after the GPIO HW is programmed as output and with the correct output value. In this scenario, the pinctrl default state wouldn't touch the pin's input/output setting; that operation would be deferred until the driver set up the GPIO as an output.
On Wed, Nov 20, 2013 at 4:16 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 11/19/2013 11:59 AM, Doug Anderson wrote: >> On Tue, Nov 19, 2013 at 10:46 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 11/19/2013 10:15 AM, Tomasz Figa wrote: >>>> This patch extends the range of settings configurable via pinfunc API >>>> to cover pin value as well. This allows configuration of default values >>>> of pins. >>> >>> Shouldn't there be a driver that acquires the GPIO that's output to the >>> pin, and configures the output value? IIRC there have been previous >>> discussions re: having a list of e.g. initial GPIO output values in DT, >>> and that was rejected, and this patch seems to be doing almost the exact >>> same thing, just at the pinctrl level rather than GPIO level. >>> >>> That all said, I admit this could be a useful feature... >> >> I haven't followed all of the previous discussions, but I know I've >> run into scenarios where something like this would be useful. The one >> that comes to mind is: >> >> * We've got GPIOs that default at bootup to a pulled up input since >> the default state of the pin should be "high". >> >> * These pins are really intended to be outputs, like an "enable", >> "reset", or "power down" line for a peripheral. The pullup is strong >> enough to give us a good default state but we really want outputs. >> >> * We'd like to provide this GPIO to a peripheral through device tree. >> ...and we'd like all the pinmux to be setup automatically so we use >> pinctrl-names = "default". >> >> * If we set the pinmux up as "output" then there's a chance that the >> line will glitch at bootup since the pinmux happens (changing the pin >> to output) before the driver has a chance to run. > > I think that last point should be addressed by having a driver that owns > the GPIO set it to the desired output level, and the implementation of Some pins are not connected (NC). At that cases, there's no drivers to handle it. To reduce power leakage, it sets proper configuration with values instead of reset values. Thank you, Kyungmin Park > the SoC's GPIO driver communicate with the pinctrl driver (which might > be the same driver; not sure here) so that gpio_direction_output() > causes the pinctrl HW to be programmed as output only after the GPIO HW > is programmed as output and with the correct output value. In this > scenario, the pinctrl default state wouldn't touch the pin's > input/output setting; that operation would be deferred until the driver > set up the GPIO as an output. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 11/19/2013 05:02 PM, Kyungmin Park wrote: > On Wed, Nov 20, 2013 at 4:16 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 11/19/2013 11:59 AM, Doug Anderson wrote: >>> On Tue, Nov 19, 2013 at 10:46 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> On 11/19/2013 10:15 AM, Tomasz Figa wrote: >>>>> This patch extends the range of settings configurable via pinfunc API >>>>> to cover pin value as well. This allows configuration of default values >>>>> of pins. >>>> >>>> Shouldn't there be a driver that acquires the GPIO that's output to the >>>> pin, and configures the output value? IIRC there have been previous >>>> discussions re: having a list of e.g. initial GPIO output values in DT, >>>> and that was rejected, and this patch seems to be doing almost the exact >>>> same thing, just at the pinctrl level rather than GPIO level. >>>> >>>> That all said, I admit this could be a useful feature... >>> >>> I haven't followed all of the previous discussions, but I know I've >>> run into scenarios where something like this would be useful. The one >>> that comes to mind is: >>> >>> * We've got GPIOs that default at bootup to a pulled up input since >>> the default state of the pin should be "high". >>> >>> * These pins are really intended to be outputs, like an "enable", >>> "reset", or "power down" line for a peripheral. The pullup is strong >>> enough to give us a good default state but we really want outputs. >>> >>> * We'd like to provide this GPIO to a peripheral through device tree. >>> ...and we'd like all the pinmux to be setup automatically so we use >>> pinctrl-names = "default". >>> >>> * If we set the pinmux up as "output" then there's a chance that the >>> line will glitch at bootup since the pinmux happens (changing the pin >>> to output) before the driver has a chance to run. >> >> I think that last point should be addressed by having a driver that owns >> the GPIO set it to the desired output level, and the implementation of > > Some pins are not connected (NC). At that cases, there's no drivers to > handle it. To reduce power leakage, it sets proper configuration with > values instead of reset values. Hmm. Shouldn't board firmware configure that kind of thing? (Of course, some firmware is starting to use DT to configure itself, so that just shifts the DT discussion, but anyway).
On Tue, Nov 19, 2013 at 05:07:04PM -0700, Stephen Warren wrote: > On 11/19/2013 05:02 PM, Kyungmin Park wrote: > > Some pins are not connected (NC). At that cases, there's no drivers to > > handle it. To reduce power leakage, it sets proper configuration with > > values instead of reset values. > Hmm. Shouldn't board firmware configure that kind of thing? > (Of course, some firmware is starting to use DT to configure itself, so > that just shifts the DT discussion, but anyway). Even if it's not sometimes the firmware is intended to be thin with configuration punted to the OS.
Hi Stephen, On Tuesday 19 of November 2013 11:46:10 Stephen Warren wrote: > On 11/19/2013 10:15 AM, Tomasz Figa wrote: > > This patch extends the range of settings configurable via pinfunc API > > to cover pin value as well. This allows configuration of default values > > of pins. > > Shouldn't there be a driver that acquires the GPIO that's output to the > pin, and configures the output value? IIRC there have been previous > discussions re: having a list of e.g. initial GPIO output values in DT, > and that was rejected, and this patch seems to be doing almost the exact > same thing, just at the pinctrl level rather than GPIO level. Well, on the contrary, I remember a discussion about specifying initial clock tree configuration in DT (on Linaro Connect in Dublin, but AFAIR also on the ML before. Through analogy, I would extend this to initial pin settins. However maybe the use cases behind this will make things clearer. > > That all said, I admit this could be a useful feature... Two specific things I had in mind with this have been: - pins of the SoC unused on particular boards, which often need different configuration depending on pin bank, SoC revision and so on. I know that _ideally_ this should be done by "firmware", but I believe we have enough historic experience to know that we shouldn't expect from the bootloader more than that it just loads us, as the name would suggest (and I have even had experience with bootloaders that couldn't properly do this basic task, screwing up, for example, by leaving MMU turned on before jumping to the kernel). - initializing safe default state of all pins on the board, to cope with configurations when not all drivers are enabled. As an example, take an embedded board with a little of flash storage, where the kernel has to be really small, so not all SoC drivers can be compiled into it. Moving the responsibility of setting initial pin states to drivers would leave pins of inexistent drivers unconfigured. I don't think I need to explain why misconfigured pins are a bad thing. Best regards, Tomasz
On Tuesday 19 of November 2013 10:59:39 Doug Anderson wrote: > On Tue, Nov 19, 2013 at 10:46 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > > On 11/19/2013 10:15 AM, Tomasz Figa wrote: > >> This patch extends the range of settings configurable via pinfunc API > >> to cover pin value as well. This allows configuration of default values > >> of pins. > > > > Shouldn't there be a driver that acquires the GPIO that's output to the > > pin, and configures the output value? IIRC there have been previous > > discussions re: having a list of e.g. initial GPIO output values in DT, > > and that was rejected, and this patch seems to be doing almost the exact > > same thing, just at the pinctrl level rather than GPIO level. > > > > That all said, I admit this could be a useful feature... > > I haven't followed all of the previous discussions, but I know I've > run into scenarios where something like this would be useful. The one > that comes to mind is: > > * We've got GPIOs that default at bootup to a pulled up input since > the default state of the pin should be "high". > > * These pins are really intended to be outputs, like an "enable", > "reset", or "power down" line for a peripheral. The pullup is strong > enough to give us a good default state but we really want outputs. > > * We'd like to provide this GPIO to a peripheral through device tree. > ...and we'd like all the pinmux to be setup automatically so we use > pinctrl-names = "default". > > * If we set the pinmux up as "output" then there's a chance that the > line will glitch at bootup since the pinmux happens (changing the pin > to output) before the driver has a chance to run. > > > Does that sound like the same scenario you're trying to solve Tomasz? Yes. That's one of the use cases I had in my mind. Best regards, Tomasz
On Wed, Nov 20, 2013 at 1:02 AM, Kyungmin Park <kmpark@infradead.org> wrote: > On Wed, Nov 20, 2013 at 4:16 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> I think that last point should be addressed by having a driver that owns >> the GPIO set it to the desired output level, and the implementation of > Some pins are not connected (NC). At that cases, there's no drivers to > handle it. To reduce power leakage, it sets proper configuration with > values instead of reset values. This is correspondant to the PIN_CONFIG_OUTPUT from include/linux/pinctrl/pinconf-generic.h I.e. driving a pin - any pin, even one that cannot do GPIO - high or low as default. One could argue that if you can drive the pin high/low using software then by definition it *is* GPIO. Even if it cannot trigger IRQs or anything. The rationale for having it in pinconf-generic is basically for use cases such that one of the the pin config states the device pass through may relate to what the documentation calls the "GPIO mode fallacy" - a state on the pins that is definately related to the use case of a certain device, but puts the pin in something the manual calls "GPIO mode" in order to save power. But from a use case point of view that is not GPIO, it is the typically the sleep state of a certain pin when used with a certain device. I'll see if I can think of some doc patch to make this more clear... Yours, Linus Walleij
On 25/11/2013 16:34, Linus Walleij wrote: > On Wed, Nov 20, 2013 at 1:02 AM, Kyungmin Park <kmpark@infradead.org> wrote: >> On Wed, Nov 20, 2013 at 4:16 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> I think that last point should be addressed by having a driver that owns >>> the GPIO set it to the desired output level, and the implementation of >> Some pins are not connected (NC). At that cases, there's no drivers to >> handle it. To reduce power leakage, it sets proper configuration with >> values instead of reset values. > This is correspondant to the PIN_CONFIG_OUTPUT from > include/linux/pinctrl/pinconf-generic.h Indeed it is - I was waiting for someone to point that out. Now we've got there... I've been working on extending the shmobile PFC driver to provide "gpio-mode" and implement PIN_CONFIG_OUTPUT as described by Documentation/pinctrl.txt, primarily to handle sleep states, but I have begun to wonder about the initial state problem, as discussed here. As far as I can see, we can't currently specify "fallback" states for pins, which is one of Tomasz' key requirements. We can specify "hog" states, and we can specify "default for a driver", but not "default before/in absence of a driver" or "sleep in absence of a driver". Having a hog precludes any finer driver control, AFAICT. Some of our existing pre-pinconf board files have a "unused pins" table which is used to claim and pull GPIOs. I've converted that to "hog" pinconf, but that only works because the table omits all pins used by drivers. And, unsurprisingly, that's been error-prone; if those tables originally covered all unused pins, they don't any more. I'd like confidence that we can get every pin into the correct state by having a fully-populated table containing all pins, that can be used regardless of which drivers start. I think what we're lacking is a "weak hog". Whatever you call that. :) That would also specify the state to fallback to when a group is released (where we currently do pinmux_disable_setting). Thoughts? Kevin
On Monday 25 of November 2013 22:01:42 Kevin Bracey wrote: > On 25/11/2013 16:34, Linus Walleij wrote: > > On Wed, Nov 20, 2013 at 1:02 AM, Kyungmin Park <kmpark@infradead.org> wrote: > >> On Wed, Nov 20, 2013 at 4:16 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > >>> I think that last point should be addressed by having a driver that owns > >>> the GPIO set it to the desired output level, and the implementation of > >> Some pins are not connected (NC). At that cases, there's no drivers to > >> handle it. To reduce power leakage, it sets proper configuration with > >> values instead of reset values. > > This is correspondant to the PIN_CONFIG_OUTPUT from > > include/linux/pinctrl/pinconf-generic.h > > Indeed it is - I was waiting for someone to point that out. Now we've > got there... > > I've been working on extending the shmobile PFC driver to provide > "gpio-mode" and implement PIN_CONFIG_OUTPUT as described by > Documentation/pinctrl.txt, primarily to handle sleep states, but I have > begun to wonder about the initial state problem, as discussed here. > > As far as I can see, we can't currently specify "fallback" states for > pins, which is one of Tomasz' key requirements. > > We can specify "hog" states, and we can specify "default for a driver", > but not "default before/in absence of a driver" or "sleep in absence of > a driver". Having a hog precludes any finer driver control, AFAICT. > > Some of our existing pre-pinconf board files have a "unused pins" table > which is used to claim and pull GPIOs. I've converted that to "hog" > pinconf, but that only works because the table omits all pins used by > drivers. And, unsurprisingly, that's been error-prone; if those tables > originally covered all unused pins, they don't any more. > > I'd like confidence that we can get every pin into the correct state by > having a fully-populated table containing all pins, that can be used > regardless of which drivers start. I think what we're lacking is a "weak > hog". Whatever you call that. :) > > That would also specify the state to fallback to when a group is > released (where we currently do pinmux_disable_setting). > > Thoughts? I fully agree with Kevin. We're currently also using "hogs" for this, but as Kevin mentioned, this is error prone, as it can be used for pins that are not touched by any drivers. IMHO a way to specify a default safe state of all pins (with lowest power consumption, without possibility of glitching external devices, etc.) would be really useful for Samsung platforms (and probably Renesas ones as Kevin wrote). Best regards, Tomasz
On Mon, Nov 25, 2013 at 9:01 PM, Kevin Bracey <kevin@bracey.fi> wrote: > I've been working on extending the shmobile PFC driver to provide > "gpio-mode" and implement PIN_CONFIG_OUTPUT as described by > Documentation/pinctrl.txt, primarily to handle sleep states, but I have > begun to wonder about the initial state problem, as discussed here. > > As far as I can see, we can't currently specify "fallback" states for pins, > which is one of Tomasz' key requirements. That depends on what you mean with "fallback states" so let's hash this out. > We can specify "hog" states, and we can specify "default for a driver", but > not "default before/in absence of a driver" or "sleep in absence of a > driver". Having a hog precludes any finer driver control, AFAICT. So the way it currently works is that hogs are usually used when there is no corresponding driver that can take care of the pins. If there are four SPI pins and no driver for that SPI block, a way to e.g. ground them is to do so using pinctrl hogs. Whenever a driver and a struct device * for this SPI block appears, it is wise to transfer control of these over to the device core, which will take any "default" state before doing probe() on the device driver, and then give you the option to use the nice pinctrl_pm* accessors to let the device core switch between "default" "idle" and "sleep" states if these are defined. > Some of our existing pre-pinconf board files have a "unused pins" table > which is used to claim and pull GPIOs. I've converted that to "hog" pinconf, > but that only works because the table omits all pins used by drivers. And, > unsurprisingly, that's been error-prone; if those tables originally covered > all unused pins, they don't any more. > > I'd like confidence that we can get every pin into the correct state by > having a fully-populated table containing all pins, that can be used > regardless of which drivers start. I think what we're lacking is a "weak > hog". Whatever you call that. :) > > That would also specify the state to fallback to when a group is released > (where we currently do pinmux_disable_setting). So I guess what you're after is a kind of hog that will be pushed aside and ignored if a struct device with an associated state appears that will use the same pin? It is true that we currently require the tables to be strict and not overlap like this, so ideally you should remove the hogs when you have a device driver, but you're actually describing an interesting case here: What if I have a driver for this IP block, and it was supposed to take care of a few pins, but I decide not to compile it into my kernel? Or if I have it as a module and only modprobe it later at runtime? So a suggested patch to support weak hogs would be interesting to look at. Can you provide details on how you think this would work? Yours, Linus Walleij
On Tue, Nov 26, 2013 at 1:30 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > IMHO a way to specify a default safe state of all pins (with lowest power > consumption, without possibility of glitching external devices, etc.) > would be really useful for Samsung platforms (and probably Renesas ones > as Kevin wrote). I'm following :-) Now if we could see some more details and a patch, that would be great ... Yours, Linus Walleij
On Tuesday 03 of December 2013 10:31:12 Linus Walleij wrote: > On Tue, Nov 26, 2013 at 1:30 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote: > > > IMHO a way to specify a default safe state of all pins (with lowest power > > consumption, without possibility of glitching external devices, etc.) > > would be really useful for Samsung platforms (and probably Renesas ones > > as Kevin wrote). > > I'm following :-) > > Now if we could see some more details and a patch, that would > be great ... Yeah, I would like to see them too. ;) Unfortunately my make_pinctrl_default_state_patches() function is returning -ENOTIME at the moment. :( (I hope to get it fixed later this month, though.) Best regards, Tomasz
On Tue, Dec 03, 2013 at 10:29:42AM +0100, Linus Walleij wrote: > So a suggested patch to support weak hogs would be interesting > to look at. Can you provide details on how you think this would > work? Or should we be going and applying the default state to all devices on init without worrying about a driver appearing?
On Thursday 05 of December 2013 15:07:47 Mark Brown wrote: > On Tue, Dec 03, 2013 at 10:29:42AM +0100, Linus Walleij wrote: > > > So a suggested patch to support weak hogs would be interesting > > to look at. Can you provide details on how you think this would > > work? > > Or should we be going and applying the default state to all devices on > init without worrying about a driver appearing? If a device isn't used, then it's often better to configure the pins for a different function, such as GPIO, to minimize leakage current. Best regards, Tomasz
On 05/12/2013 17:11, Tomasz Figa wrote: > On Thursday 05 of December 2013 15:07:47 Mark Brown wrote: >> On Tue, Dec 03, 2013 at 10:29:42AM +0100, Linus Walleij wrote: >> >>> So a suggested patch to support weak hogs would be interesting >>> to look at. Can you provide details on how you think this would >>> work? >> Or should we be going and applying the default state to all devices on >> init without worrying about a driver appearing? > If a device isn't used, then it's often better to configure the pins for > a different function, such as GPIO, to minimize leakage current. > And there can also be mutually-exclusive drivers choosing different default states for the same pin. I think you do need a separate "safe" indicator. My current thought is that a late-init "make safe all unclaimed pins" pass would make sense - you can't really mess with pins in an automated fashion on init, as it can mess up bootloader->driver handover. There already exist late-init "turn off all unclaimed clocks" (at least on shmobile) and "turn off all unclaimed regulators", and it would fit that model. Kevin
On Thursday 05 of December 2013 18:49:56 Kevin Bracey wrote: > On 05/12/2013 17:11, Tomasz Figa wrote: > > On Thursday 05 of December 2013 15:07:47 Mark Brown wrote: > >> On Tue, Dec 03, 2013 at 10:29:42AM +0100, Linus Walleij wrote: > >> > >>> So a suggested patch to support weak hogs would be interesting > >>> to look at. Can you provide details on how you think this would > >>> work? > >> Or should we be going and applying the default state to all devices on > >> init without worrying about a driver appearing? > > If a device isn't used, then it's often better to configure the pins for > > a different function, such as GPIO, to minimize leakage current. > > > > And there can also be mutually-exclusive drivers choosing different > default states for the same pin. I think you do need a separate "safe" > indicator. That's not quite true, as on a single board you should rather have a single device node with "okay" status referencing given set of pins. Still, I think that a separate safe state is the way to go. > > My current thought is that a late-init "make safe all unclaimed pins" > pass would make sense - you can't really mess with pins in an automated > fashion on init, as it can mess up bootloader->driver handover. There > already exist late-init "turn off all unclaimed clocks" (at least on > shmobile) That's a feature of Common Clock Framework. > and "turn off all unclaimed regulators", and it would fit that > model. Maybe that's the way to do it. I need to think a bit more on this, especially considering our (Samsung's) use cases. Best regards, Tomasz
On Thu, Dec 05, 2013 at 04:11:15PM +0100, Tomasz Figa wrote: > On Thursday 05 of December 2013 15:07:47 Mark Brown wrote: > > Or should we be going and applying the default state to all devices on > > init without worrying about a driver appearing? > If a device isn't used, then it's often better to configure the pins for > a different function, such as GPIO, to minimize leakage current. That's true but on the other hand it's much more likely that the default information will be there than the hog for the case when the driver doesn't get loaded for some reason so it's more likely that the default will kick in; perhaps we need to work down a list of states in order instead?
On 12/03/2013 02:29 AM, Linus Walleij wrote: ... > So I guess what you're after is a kind of hog that will be pushed > aside and ignored if a struct device with an associated state appears > that will use the same pin? That probably would be useful. Perhaps we should just make all hogs not exclusively own the pins they configure? > It is true that we currently require the tables to be strict and not > overlap like this, so ideally you should remove the hogs when you > have a device driver, but you're actually describing an interesting > case here: > > What if I have a driver for this IP block, and it was supposed to > take care of a few pins, but I decide not to compile it into my > kernel? Or if I have it as a module and only modprobe it later > at runtime? Indeed, that's the nasty hole in pushing even static per-device pinctrl configuration into each device's node; the device may not appear. Related, I prefer to put /all/ static pinctrl configuration into the pinctrl device's "default" state (i.e. use a hog) rather than configuring the static pinctrl setup per device, for another reason too: If a particular IO controller's signals can be routed to n different (sets of) pins, then we need to do *both* of the following when setting up the pinmux: a) Configure the pins we want to host those signals to route to/from that particular IO controller. b) Configure any other pins that could route to/from that particular IO controller as some other function; either disabled, or routed to/from some different IO controller. That is so that the IO controller's RX/input signals are not connected from two different sets of pins at once, which would cause two things to driver them. Depending on HW, this could cause on of: 1) Multiple drivers -> high power usage, or even Silicon damage. 2) Inconsistent configuration, with the "wrong" set of pins driving the IO controller's inputs, and hence the signals on the "correct" pins being ignored -> hard to find bug. Now, (a) could easily happen when the driver for the IO controller is probed. However, (b) can't, because some other IO controller may need to use those pins, and the two drivers (or pinctrl states for different devices) can't both set up those pins. The only way to solve this is to set up all pinmux state using a single global table (e.g. the pin controller's default state, or hog) that is applied early on. If we rely on resolving these conflicts in per-device default/... states, then that means the conflicts won't get resolved until a driver gets probed, if it ever does, which is too late. ... and as such, I prefer only putting *dynamic* configuration into per-device (non-hog) nodes. (e.g. an I2C bus mux driver which actively changes the pinmux at run-time to move an I2C controller between different sets of SoC pins).
On Thu, Dec 5, 2013 at 4:07 PM, Mark Brown <broonie@kernel.org> wrote: > On Tue, Dec 03, 2013 at 10:29:42AM +0100, Linus Walleij wrote: > >> So a suggested patch to support weak hogs would be interesting >> to look at. Can you provide details on how you think this would >> work? > > Or should we be going and applying the default state to all devices on > init without worrying about a driver appearing? That doesn't really work: if you have an unused device not in the DT you usually mark it "disabled", and then the DT core doesn't even create a platform_device for this node. So doing this would involve parsing the tree and ... yuck. Yours, Linus Walleij
On Fri, Dec 6, 2013 at 12:54 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 12/03/2013 02:29 AM, Linus Walleij wrote: (skipped the conversation on weak hogs, we are on the same page here, just waiting for someone to start working on it ...) > Related, I prefer to put /all/ static pinctrl configuration into the > pinctrl device's "default" state (i.e. use a hog) rather than > configuring the static pinctrl setup per device, for another reason too: > > If a particular IO controller's signals can be routed to n different > (sets of) pins, then we need to do *both* of the following when setting > up the pinmux: > > a) Configure the pins we want to host those signals to route to/from > that particular IO controller. > > b) Configure any other pins that could route to/from that particular IO > controller as some other function; either disabled, or routed to/from > some different IO controller. > > That is so that the IO controller's RX/input signals are not connected > from two different sets of pins at once, which would cause two things to > driver them. Depending on HW, this could cause on of: > > 1) Multiple drivers -> high power usage, or even Silicon damage. > > 2) Inconsistent configuration, with the "wrong" set of pins driving the > IO controller's inputs, and hence the signals on the "correct" pins > being ignored -> hard to find bug. I'm following, I think what we need here is to think about additional behaviours and electronic constraints we can encode into the drivers and/or the pin tables to safeguard pin states from electronically unsound states. That is to say, I prefer the subsystem to be conscious about the electronic constraints and navigate around them or put up a road block, rather than trying ti avoid driving into the roadblocks by means of carefully crafted tables if you get the picture ... Yours, Linus Walleij
On Mon, Dec 09, 2013 at 11:22:44AM +0100, Linus Walleij wrote: > On Thu, Dec 5, 2013 at 4:07 PM, Mark Brown <broonie@kernel.org> wrote: > > Or should we be going and applying the default state to all devices on > > init without worrying about a driver appearing? > That doesn't really work: if you have an unused device not in the > DT you usually mark it "disabled", and then the DT core doesn't > even create a platform_device for this node. > So doing this would involve parsing the tree and ... yuck. Hrm, yeah, good point. That's got a low awesomeness factor even though it should work. Boo.
diff --git a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt index fe34cbb..ed4cc9c 100644 --- a/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt +++ b/Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt @@ -74,6 +74,7 @@ Required Properties: "samsung,pins" property of the child node. The following pin configuration properties are supported. + - samsung,pin-val: Initial value of pin output buffer. - samsung,pin-pud: Pull up/down configuration. - samsung,pin-drv: Drive strength configuration. - samsung,pin-pud-pdn: Pull up/down configuration in power down mode. diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c index c752de4..6b2194a 100644 --- a/drivers/pinctrl/pinctrl-samsung.c +++ b/drivers/pinctrl/pinctrl-samsung.c @@ -47,6 +47,7 @@ static struct pin_config { { "samsung,pin-drv", PINCFG_TYPE_DRV }, { "samsung,pin-con-pdn", PINCFG_TYPE_CON_PDN }, { "samsung,pin-pud-pdn", PINCFG_TYPE_PUD_PDN }, + { "samsung,pin-val", PINCFG_TYPE_DAT }, }; /* Global list of devices (struct samsung_pinctrl_drv_data) */