[PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Tomasz Figa]

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.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt | 1 +
 drivers/pinctrl/pinctrl-samsung.c                             | 1 +
 2 files changed, 2 insertions(+)

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) */
-- 
1.8.4.2

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Stephen Warren]

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...

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Doug Anderson]

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?

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Stephen Warren]

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.

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Kyungmin Park]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Stephen Warren]

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).

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Tomasz Figa]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Tomasz Figa]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Linus Walleij]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Kevin Bracey]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Tomasz Figa]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Linus Walleij]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Tomasz Figa]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 323 bytes --]

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Tomasz Figa]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Kevin Bracey]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Tomasz Figa]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Stephen Warren]

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).

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Linus Walleij]

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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[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

Re: [PATCH] pinctrl: samsung: Allow pin value to be initialized using pinfunc.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 603 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]