Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1370946 > unrolled thread

Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Mark Rutland <mark.rutland@arm.com> - 2016-04-05 01:00 +0200
    Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Linus Walleij <linus.walleij@linaro.org> - 2016-04-05 10:50 +0200
      Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Mark Brown <broonie@kernel.org> - 2016-04-05 19:00 +0200
        Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Octavian Purdila <octavian.purdila@intel.com> - 2016-04-05 21:40 +0200
          Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Mark Brown <broonie@kernel.org> - 2016-04-06 00:50 +0200
          Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Al Stone <astone@redhat.com> - 2016-04-06 01:50 +0200
          Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2016-04-06 11:00 +0200
    RE: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration "Tirdea, Irina" <irina.tirdea@intel.com> - 2016-04-05 17:40 +0200
      Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Mark Rutland <mark.rutland@arm.com> - 2016-04-05 20:30 +0200
        Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Octavian Purdila <octavian.purdila@intel.com> - 2016-04-05 22:10 +0200
          Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Mark Rutland <mark.rutland@arm.com> - 2016-04-06 02:10 +0200
            Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Octavian Purdila <octavian.purdila@intel.com> - 2016-04-07 14:20 +0200
              Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Mark Rutland <mark.rutland@arm.com> - 2016-04-07 18:00 +0200
                Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-04-07 23:30 +0200
                  Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Mark Brown <broonie@kernel.org> - 2016-04-12 14:20 +0200
                    Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration "Rafael J. Wysocki" <rafael@kernel.org> - 2016-04-13 07:10 +0200
    Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Al Stone <astone@redhat.com> - 2016-04-06 02:10 +0200
      Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Graeme Gregory <gg@slimlogic.co.uk> - 2016-04-06 12:50 +0200
        Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Octavian Purdila <octavian.purdila@intel.com> - 2016-04-07 16:20 +0200
          Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Linus Walleij <linus.walleij@linaro.org> - 2016-04-07 20:10 +0200

#1370946 — Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration

Hi,

On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
> This is a proposal for adding ACPI support for pin controller
> configuration.
> 
> It has been developed to enable the MinnowBoard and IoT community
> by providing an easy way to specify pin multiplexing and
> pin configuration.
> 
> This proposal is based on using _DSD properties to specify device
> states and configuration nodes and it follows closely the device
> tree model. Device states are defined using the Device Properties
> format and the configuration nodes are defined using the
> Hierarchical Properties Extension format. The generic properties
> for the configuration nodes are the same as the ones for device
> tree, while pincontroller drivers can also define custom ones.

From a look of the Documentation addition, and of the current uses of
pinctrl-names in device tree bindings, one reason for requiring multiple
pinctrl states is power management. Given that, I'm somewhat concerned by this,
as it goes against the usual ACPI model of abstracting this sort of thing
behind power control methods.

To the best of my knowledge, that goes against the ASWG's expectations on how
_DSD will be used (per [1]). Charles, please correct me if that document is no
longer representative.

Additionally, pinctrl has some cross-cutting concerns (e.g. mutually exclusive
device usage due to a shared pin), and I can imagine that may interact poorly
with any AML or firmware assumptions about the state of the world, as there's
no mechanism present to notify those of changes to pins.

I think that this is a class of problem which needs to be raised with the ASWG,
and solved in an ACPI-native fashion, rather than simply copying properties
from DT using _DSD. If nothing else, the restrictions on FW and AML would need
to be specified.

Thanks,
Mark.

[1] https://lists.acpica.org/pipermail/dsd/2015-September/000019.html

[toc] | [next] | [standalone]

#1371322

On Tue, Apr 5, 2016 at 12:52 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:

>> This proposal is based on using _DSD properties to specify device
>> states and configuration nodes and it follows closely the device
>> tree model. Device states are defined using the Device Properties
>> format and the configuration nodes are defined using the
>> Hierarchical Properties Extension format. The generic properties
>> for the configuration nodes are the same as the ones for device
>> tree, while pincontroller drivers can also define custom ones.
>
> From a look of the Documentation addition, and of the current uses of
> pinctrl-names in device tree bindings, one reason for requiring multiple
> pinctrl states is power management. Given that, I'm somewhat concerned by this,
> as it goes against the usual ACPI model of abstracting this sort of thing
> behind power control methods.

There are other use cases but in essence there are two common use
cases:

- Initial set-up of muxing and electrical properties

- Runtime reconfiguration for power saving (especially
  biasing and pull options)

I did see that the ACPI people's ambition has been for all things
power save to be embedded into AML and the like.

However AFAICT
the development model for deployment of ACPI seems to imply that
products are shipped with ACPI tables resident in ROM-like storage,
and at product release several devices are disabled from muxing
while others at the same time are totally lacking power save
enablement.

And these products are only update-able
with hairy BIOS patches that need to be applied
using $SPECIAL_TOOL  that "normal users" do not want to
concern themselves with, as this is not an "apt-get upgrade"
kind of thing.

And as this is server silicon the systems have a bunch of these
"normal users" that are not embedded development experts
and they are very hesitant about doing such things.

Under that scenario it is of course tempting to simply set up the
pins in a known good state and then remove
the responsibility of pin controlling from ACPI firmware altogether
and into the kernel, where "apt-get upgrade" will take care of
post-product launch upgrading of the functionality.

And I think that is what is happening, it's just that so much
prestige is involved that no-one wants to officially admit it.

I was once poking fun at this development model accusing
firmware engineers of having a God complex when they claim
to be able to engineer in these complex use cases during
product firmware development. Now I just feel sad about this
situation and want things to "just work" for them. I think these
patches are good.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1371791

[Multipart message — attachments visible in raw view] — view raw

On Tue, Apr 05, 2016 at 10:43:11AM +0200, Linus Walleij wrote:

> And these products are only update-able
> with hairy BIOS patches that need to be applied
> using $SPECIAL_TOOL  that "normal users" do not want to
> concern themselves with, as this is not an "apt-get upgrade"
> kind of thing.

This bit has been addressed in UEFI - there's now a mechanism for the OS
to supply firmware updates to the BIOS via runtime sevices calls without
needing to go to the hairy tools.  There's also been facilities for some
time which allow ACPI fragments to be loaded at runtime without patching
the firmware (though they're not used so often at present).

> And I think that is what is happening, it's just that so much
> prestige is involved that no-one wants to officially admit it.

> I was once poking fun at this development model accusing
> firmware engineers of having a God complex when they claim
> to be able to engineer in these complex use cases during

That's not really a fair characterization of the situation.  The goal
that ACPI has been trying to meet is allowing new hardware to work
without needing software updates so people's installation experience
isn't miserable.  The Windows monoculture that firmware developers have
been targetting has hurt the achievement of that but it's the idea.
It's a good model for some classes of device but not for all.

> product firmware development. Now I just feel sad about this
> situation and want things to "just work" for them. I think these
> patches are good.

See Mark Rutland's reply - there's a whole model behind how ACPI
abstracts the system that needs to be taken into account, just stuffing
the existing DT in through a mechanism intended for simple vendor
specific key/value properties isn't guaranteed to give something that's
coherent and sensible.  DT design decisions will obviously not have
considered ACPI and exposing the full combination of the two system
models to system integrators seems likely to lead to unfortunate
corners, especially with things that happen to work right now but cause
problems later on.

Given the rush to shoehorn existing DT into ACPI at some point it's
looking like it would be much more sensible for x86 to just do what
arm64 has done and support both DT and ACPI in parallel and let people
(either system integrators or end users) choose the most appropriate
interface for their application.

[toc] | [prev] | [next] | [standalone]

#1371904

On Tue, Apr 5, 2016 at 7:59 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Apr 05, 2016 at 10:43:11AM +0200, Linus Walleij wrote:
>
>> And these products are only update-able
>> with hairy BIOS patches that need to be applied
>> using $SPECIAL_TOOL  that "normal users" do not want to
>> concern themselves with, as this is not an "apt-get upgrade"
>> kind of thing.
>
> This bit has been addressed in UEFI - there's now a mechanism for the OS
> to supply firmware updates to the BIOS via runtime sevices calls without
> needing to go to the hairy tools.

AFAIK there is still no standard way of updating just the ACPI tables.
You need various tools to "stitch" a full firmware image that can be
consumed by the update mechanism.

> There's also been facilities for some
> time which allow ACPI fragments to be loaded at runtime without patching
> the firmware (though they're not used so often at present).

And I just sent a separate patch series for "ACPI overlays", similar
with the dynamic DT, with the primary goal of supporting open-ended
hardware configurations.

But to be able to effectively use this we need a way to change pinmux settings.

>> And I think that is what is happening, it's just that so much
>> prestige is involved that no-one wants to officially admit it.
>
>> I was once poking fun at this development model accusing
>> firmware engineers of having a God complex when they claim
>> to be able to engineer in these complex use cases during
>
> That's not really a fair characterization of the situation.  The goal
> that ACPI has been trying to meet is allowing new hardware to work
> without needing software updates so people's installation experience
> isn't miserable.  The Windows monoculture that firmware developers have
> been targetting has hurt the achievement of that but it's the idea.
> It's a good model for some classes of device but not for all.
>
>> product firmware development. Now I just feel sad about this
>> situation and want things to "just work" for them. I think these
>> patches are good.
>
> See Mark Rutland's reply - there's a whole model behind how ACPI
> abstracts the system that needs to be taken into account, just stuffing
> the existing DT in through a mechanism intended for simple vendor
> specific key/value properties isn't guaranteed to give something that's
> coherent and sensible.  DT design decisions will obviously not have
> considered ACPI and exposing the full combination of the two system
> models to system integrators seems likely to lead to unfortunate
> corners, especially with things that happen to work right now but cause
> problems later on.
>
> Given the rush to shoehorn existing DT into ACPI at some point it's
> looking like it would be much more sensible for x86 to just do what
> arm64 has done and support both DT and ACPI in parallel and let people
> (either system integrators or end users) choose the most appropriate
> interface for their application.

Lets look at this from a different perspective. The proposal is not
about importing the DT model into ACPI but importing the Linux pinctrl
model into ACPI. That will allow us to use the Linux pinctrl drivers
to their full potential.

That doesn't stop the development of other, more OS independent, ACPI
models for pinmuxing. Which we can also support.

I know that there are some discussions for pinmux configuration in the
ASWG, but it does not match the Linux pinctrl model. So we will end up
with a pinctrl driver that offers groups, functions and pin names and
a totally different ACPI description that we can't map to the pinctrl
driver.

[toc] | [prev] | [next] | [standalone]

#1372045

[Multipart message — attachments visible in raw view] — view raw

On Tue, Apr 05, 2016 at 10:37:14PM +0300, Octavian Purdila wrote:
> On Tue, Apr 5, 2016 at 7:59 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Apr 05, 2016 at 10:43:11AM +0200, Linus Walleij wrote:

> >> And these products are only update-able
> >> with hairy BIOS patches that need to be applied
> >> using $SPECIAL_TOOL  that "normal users" do not want to
> >> concern themselves with, as this is not an "apt-get upgrade"
> >> kind of thing.

> > This bit has been addressed in UEFI - there's now a mechanism for the OS
> > to supply firmware updates to the BIOS via runtime sevices calls without
> > needing to go to the hairy tools.

> AFAIK there is still no standard way of updating just the ACPI tables.
> You need various tools to "stitch" a full firmware image that can be
> consumed by the update mechanism.

Right, but in terms of end users that doesn't matter - the users that
Linus is concerned with here aren't going to know what ACPI is.

> > There's also been facilities for some
> > time which allow ACPI fragments to be loaded at runtime without patching
> > the firmware (though they're not used so often at present).

> And I just sent a separate patch series for "ACPI overlays", similar
> with the dynamic DT, with the primary goal of supporting open-ended
> hardware configurations.

> But to be able to effectively use this we need a way to change pinmux settings.

I'll need to find that series I guess...

Note that we currently don't have any actual mechanism to load a DT
overlay in mainline.  One of the issues there is making sure that the
external interfaces we're offering are actually stable, another is
making sure (for safety) that the overlays are only updating bits of the
device tree that corrspond to the bit of the hardware that the overlay
is for.

> > Given the rush to shoehorn existing DT into ACPI at some point it's
> > looking like it would be much more sensible for x86 to just do what
> > arm64 has done and support both DT and ACPI in parallel and let people
> > (either system integrators or end users) choose the most appropriate
> > interface for their application.

> Lets look at this from a different perspective. The proposal is not
> about importing the DT model into ACPI but importing the Linux pinctrl

It's not like this is the only change that has been sent to reuse some
DT element rather directly in ACPI recently...

> model into ACPI. That will allow us to use the Linux pinctrl drivers
> to their full potential.

> That doesn't stop the development of other, more OS independent, ACPI
> models for pinmuxing. Which we can also support.

That's not really a great answer.  Regardless of how good a solution
this is for handling pinmuxing ACPI it needs to be joined up with the
rest of ACPI, we need some clear instructions for both firmware and OS
authors about how this works in a wider ACPI context and interacts with
other features, preferably in the ACPI specs where people can find them.

It could be something really simple like just saying that if we've got
these pinctrl definitions then the firmware is not allowed to do any pin
management, or it could be something more involved like saying that the
OS has to notify the OS before using them.

> I know that there are some discussions for pinmux configuration in the
> ASWG, but it does not match the Linux pinctrl model. So we will end up
> with a pinctrl driver that offers groups, functions and pin names and
> a totally different ACPI description that we can't map to the pinctrl
> driver.

If there's an ASWG spec in progress that explicitly overlaps it seems
even more important that this is joined up - I'm not aware of what's
going on there.

[toc] | [prev] | [next] | [standalone]

#1372067

On 04/05/2016 01:37 PM, Octavian Purdila wrote:
> On Tue, Apr 5, 2016 at 7:59 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Tue, Apr 05, 2016 at 10:43:11AM +0200, Linus Walleij wrote:
>>[snip...]
> 
> I know that there are some discussions for pinmux configuration in the
> ASWG, but it does not match the Linux pinctrl model. So we will end up
> with a pinctrl driver that offers groups, functions and pin names and
> a totally different ACPI description that we can't map to the pinctrl
> driver.

Um, as a member of the ASWG, this is the first I've heard of this discussion.
And I do attend the meetings very regularly.  Please send me an issue number
off-line because I'm not finding it in the current issue list.

Secondly, if this is being discussed within the ASWG, please refer to the UEFI
member agreements about disclosing information prior to it being approved by
the forum; there are limits to such disclosures, and a public mailing list may
be well past those limits.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

[toc] | [prev] | [next] | [standalone]

#1372301

On Tue, Apr 05, 2016 at 10:37:14PM +0300, Octavian Purdila wrote:
> On Tue, Apr 5, 2016 at 7:59 PM, Mark Brown <broonie@kernel.org> wrote:

[...]

> Lets look at this from a different perspective. The proposal is not
> about importing the DT model into ACPI but importing the Linux pinctrl
> model into ACPI. That will allow us to use the Linux pinctrl drivers
> to their full potential.

Yes we understood that, and in the process you are bypassing the eg ACPI
power management model completely, but since all you are after is using
the Linux pinctrl kernel driver with ACPI _today_ without going through
the ASWG (and without booting with a device tree instead of ACPI) and
define a specification that has a chance to co-exist with the ACPI power
management model this proposal is the end result, it is a shortcut fraught
with problems.

> That doesn't stop the development of other, more OS independent, ACPI
> models for pinmuxing. Which we can also support.
> 
> I know that there are some discussions for pinmux configuration in the
> ASWG, but it does not match the Linux pinctrl model. So we will end up
> with a pinctrl driver that offers groups, functions and pin names and
> a totally different ACPI description that we can't map to the pinctrl
> driver.

If you know that there are some discussions please take place in those
discussions and work towards a solution that takes into account
other parts of ACPI specifications that can be affected, it may
take longer to get you there but that's true for everyone who
wants to contribute to ACPI specifications I am afraid.

Thank you,
Lorenzo

[toc] | [prev] | [next] | [standalone]

#1371725

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: 05 April, 2016 1:52
> To: Tirdea, Irina
> Cc: Rafael J. Wysocki; Len Brown; Mika Westerberg; Linus Walleij; linux-gpio@vger.kernel.org; linux-acpi@vger.kernel.org; Rob
> Herring; Heikki Krogerus; Andy Shevchenko; Purdila, Octavian; Ciocan, Cristina; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; charles.garcia-tobin@arm.com
> Subject: Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
> 
> Hi,

Hi Mark,

> 
> On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
> > This is a proposal for adding ACPI support for pin controller
> > configuration.
> >
> > It has been developed to enable the MinnowBoard and IoT community
> > by providing an easy way to specify pin multiplexing and
> > pin configuration.
> >
> > This proposal is based on using _DSD properties to specify device
> > states and configuration nodes and it follows closely the device
> > tree model. Device states are defined using the Device Properties
> > format and the configuration nodes are defined using the
> > Hierarchical Properties Extension format. The generic properties
> > for the configuration nodes are the same as the ones for device
> > tree, while pincontroller drivers can also define custom ones.
> 
> From a look of the Documentation addition, and of the current uses of
> pinctrl-names in device tree bindings, one reason for requiring multiple
> pinctrl states is power management. Given that, I'm somewhat concerned by this,
> as it goes against the usual ACPI model of abstracting this sort of thing
> behind power control methods.
> 

Right, there is an overlap of the pinctrl "sleep" state with the ACPI power
management model. 

However, the main reason for implementing this is setting initial pin multiplexing
and configuration. This is normally done by BIOS, but there is currently no way of
changing the default configuration (except for a BIOS update). This is a problem
when using boards like MinnowBoard, where it is expected to get the board and
to be able to add various devices to its exported GPIO pins. We need a way to
change default pin multiplexing to enable the functionality required by a specific
device. In some cases we also need to set the bias to get things like interrupts working.

Another use case for pinctrl states is using custom reset pin configurations that need
to be controlled from the driver.

Since we have the pinctrl infrastructure and the all Intel ACPI pinctrl drivers already
support it, this seems the most straightforward way to implement it.

> To the best of my knowledge, that goes against the ASWG's expectations on how
> _DSD will be used (per [1]). Charles, please correct me if that document is no
> longer representative.
> 
> Additionally, pinctrl has some cross-cutting concerns (e.g. mutually exclusive
> device usage due to a shared pin), and I can imagine that may interact poorly
> with any AML or firmware assumptions about the state of the world, as there's
> no mechanism present to notify those of changes to pins.
> 

This is a problem with any modification we would make to the ACPI tables. 
It is true that pinctrl configuration could create conflicts when changing
pin multiplexing, but that could be avoided by using only the pinctrl states that
make sense for the entire ACPI configuration.

> I think that this is a class of problem which needs to be raised with the ASWG,
> and solved in an ACPI-native fashion, rather than simply copying properties
> from DT using _DSD. If nothing else, the restrictions on FW and AML would need
> to be specified.

I agree this should be at least mentioned in the Documentation. I'll update it accordingly.


Thanks,
Irina

> 
> Thanks,
> Mark.
> 
> [1] https://lists.acpica.org/pipermail/dsd/2015-September/000019.html

[toc] | [prev] | [next] | [standalone]

#1371878

On Tue, Apr 05, 2016 at 03:33:43PM +0000, Tirdea, Irina wrote:
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
> > > This is a proposal for adding ACPI support for pin controller
> > > configuration.
> > >
> > > It has been developed to enable the MinnowBoard and IoT community
> > > by providing an easy way to specify pin multiplexing and
> > > pin configuration.
> > >
> > > This proposal is based on using _DSD properties to specify device
> > > states and configuration nodes and it follows closely the device
> > > tree model. Device states are defined using the Device Properties
> > > format and the configuration nodes are defined using the
> > > Hierarchical Properties Extension format. The generic properties
> > > for the configuration nodes are the same as the ones for device
> > > tree, while pincontroller drivers can also define custom ones.
> > 
> > From a look of the Documentation addition, and of the current uses of
> > pinctrl-names in device tree bindings, one reason for requiring multiple
> > pinctrl states is power management. Given that, I'm somewhat concerned by this,
> > as it goes against the usual ACPI model of abstracting this sort of thing
> > behind power control methods.
> > 
> 
> Right, there is an overlap of the pinctrl "sleep" state with the ACPI power
> management model. 
> 
> However, the main reason for implementing this is setting initial pin multiplexing
> and configuration. This is normally done by BIOS, but there is currently no way of
> changing the default configuration (except for a BIOS update). This is a problem
> when using boards like MinnowBoard, where it is expected to get the board and
> to be able to add various devices to its exported GPIO pins.

In the absence of a BIOS update, how is it expected that the relevant pinctrl
settings for a given device will be known? Does the device provide ACPI
fragments like an SSDT? Does it simply identify itself in some manner, and
leave the rest to the kernel? Is this entirely user-driven?

> We need a way to change default pin multiplexing to enable the functionality
> required by a specific device. In some cases we also need to set the bias to
> get things like interrupts working.
> 
> Another use case for pinctrl states is using custom reset pin configurations that need
> to be controlled from the driver.

To be clear, I'm not stating that having pinctrl under the OS is necessarily
wrong, and I can see why the firmware may not have all the relevant knowledge
in advance. I can certainly see why having the OS in control can be preferable.

My concern is that there is a conflict with the ACPI model, and potential
fragility, given that:

* The firmware does not have the relevant information in advance for a given
  device that may be connected (i.e. how devices may change the pinctrl setup
  is unknown).

* The firmware is to some extent expected to manage pinctrl today (for power
  management of devices it does know about), and hence a pinctrl device is
  potentially under shared management of ACPI and the OS.

* The ACPI specification says nothing about this style of pinctrl management,
  so it is unclear what the expectations are:

  - Is a given pinctrl device under shared ownership by the firmware and
    kernel, or is a given device entirely under the control of just one?

  - How shared access to the pinctrl device is mediated, e.g. is any locking or
    signalling mechanism required to ensure that firmware and kernel do not
    access the device simultaneously in a manner that causes problems.

  - Is the firmware permitted to perform power management of devices for which
    the kernel handles pinctrl? What states can either expect, and when is such
    management permitted? e.g. must the OS ensure that a device is in its
    default state? Can it only call power management calls from particular
    states? What is the restored state?

  - What the expectations are w.r.t. ownership of pins, e.g. must the firmware
    never change the state of certain pins? Must it save/restore their state in
    system-wide power-management scenarios like suspend or hibernate?

I think this needs to be raised with the ASWG, and some level of model needs to
be defined for this, to cater for the issues raised above.

That might be very simple, e.g. pins are never shared, the pinctrl management
device must permit concurrent accesses by FW and kernel, the kernel is
responsible for all management of those pins after system reset.

In the absence of that, this is liable to become fragmented and fragile, and is
practically impossible to rectify post-hoc.

> Since we have the pinctrl infrastructure and the all Intel ACPI pinctrl drivers already
> support it, this seems the most straightforward way to implement it.

I do not disagree that this is expedient. I am concerned that it permits a very
large set of avoidable problems, and I believe that we need to be somewhat
stricter from the outset.

> > Additionally, pinctrl has some cross-cutting concerns (e.g. mutually exclusive
> > device usage due to a shared pin), and I can imagine that may interact poorly
> > with any AML or firmware assumptions about the state of the world, as there's
> > no mechanism present to notify those of changes to pins.
> 
> This is a problem with any modification we would make to the ACPI tables. 
> It is true that pinctrl configuration could create conflicts when changing
> pin multiplexing, but that could be avoided by using only the pinctrl states that
> make sense for the entire ACPI configuration.

I think that "makes sense for the entire ACPI configuration" needs to be
defined, and then mandated, which implies an ACPI spec addition.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1371915

On Tue, Apr 5, 2016 at 9:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:

>>
>> Right, there is an overlap of the pinctrl "sleep" state with the ACPI power
>> management model.
>>
>> However, the main reason for implementing this is setting initial pin multiplexing
>> and configuration. This is normally done by BIOS, but there is currently no way of
>> changing the default configuration (except for a BIOS update). This is a problem
>> when using boards like MinnowBoard, where it is expected to get the board and
>> to be able to add various devices to its exported GPIO pins.
>
> In the absence of a BIOS update, how is it expected that the relevant pinctrl
> settings for a given device will be known? Does the device provide ACPI
> fragments like an SSDT? Does it simply identify itself in some manner, and
> leave the rest to the kernel? Is this entirely user-driven?
>

See this patch set I just proposed: https://lkml.org/lkml/2016/3/31/334

>> We need a way to change default pin multiplexing to enable the functionality
>> required by a specific device. In some cases we also need to set the bias to
>> get things like interrupts working.
>>
>> Another use case for pinctrl states is using custom reset pin configurations that need
>> to be controlled from the driver.
>
> To be clear, I'm not stating that having pinctrl under the OS is necessarily
> wrong, and I can see why the firmware may not have all the relevant knowledge
> in advance. I can certainly see why having the OS in control can be preferable.
>
> My concern is that there is a conflict with the ACPI model, and potential
> fragility, given that:
>
> * The firmware does not have the relevant information in advance for a given
>   device that may be connected (i.e. how devices may change the pinctrl setup
>   is unknown).
>
> * The firmware is to some extent expected to manage pinctrl today (for power
>   management of devices it does know about), and hence a pinctrl device is
>   potentially under shared management of ACPI and the OS.
>
> * The ACPI specification says nothing about this style of pinctrl management,
>   so it is unclear what the expectations are:
>

Does it say anything at all about pinctrl management?

>   - Is a given pinctrl device under shared ownership by the firmware and
>     kernel, or is a given device entirely under the control of just one?
>
>   - How shared access to the pinctrl device is mediated, e.g. is any locking or
>     signalling mechanism required to ensure that firmware and kernel do not
>     access the device simultaneously in a manner that causes problems.
>
>   - Is the firmware permitted to perform power management of devices for which
>     the kernel handles pinctrl? What states can either expect, and when is such
>     management permitted? e.g. must the OS ensure that a device is in its
>     default state? Can it only call power management calls from particular
>     states? What is the restored state?
>
>   - What the expectations are w.r.t. ownership of pins, e.g. must the firmware
>     never change the state of certain pins? Must it save/restore their state in
>     system-wide power-management scenarios like suspend or hibernate?
>
> I think this needs to be raised with the ASWG, and some level of model needs to
> be defined for this, to cater for the issues raised above.
>
> That might be very simple, e.g. pins are never shared, the pinctrl management
> device must permit concurrent accesses by FW and kernel, the kernel is
> responsible for all management of those pins after system reset.
>
> In the absence of that, this is liable to become fragmented and fragile, and is
> practically impossible to rectify post-hoc.
>

All very good points.

Since our focus is for open-ended configurations and for hardware that
it is not know to firmware we only considered the case where the pins
are not touched after the system boots.

Now I wonder what are the cases were the firmware changes the pin
configuration after boot.

[toc] | [prev] | [next] | [standalone]

#1372086

On Tue, Apr 05, 2016 at 11:09:34PM +0300, Octavian Purdila wrote:
> On Tue, Apr 5, 2016 at 9:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > * The firmware is to some extent expected to manage pinctrl today (for power
> >   management of devices it does know about), and hence a pinctrl device is
> >   potentially under shared management of ACPI and the OS.
> >
> > * The ACPI specification says nothing about this style of pinctrl management,
> >   so it is unclear what the expectations are:
> 
> Does it say anything at all about pinctrl management?

To the best of my knowledge the ACPI spec does not explicitly mention pinctrl.
In another reply it was mentioned that there is work in progress for pinmuxing,
so evidently it is on the ASWG radar and within the scope of ACPI.

I was trying to point out that it is _implicitly_ firmware's responsibility to
do any pinctrl today, due to the ACPI model for power management, and the lack
of an existing ACPI mechanisms to provide pinctrl data.

In practice, firmware configures pinctrl at boot, and may modify pinctrl as
part of the runtime power management firmware is put in charge of.

The lack of any statement about pinctrl would mean that there is effectively no
reasonable limitation on what firmware might do with pinctrl, and we cannot
assume specific behaviour from the firmware.

[...]

> Since our focus is for open-ended configurations and for hardware that
> it is not know to firmware we only considered the case where the pins
> are not touched after the system boots.
> 
> Now I wonder what are the cases were the firmware changes the pin
> configuration after boot.

Device power management and suspend/resume seem like the obvious cases.

Thanks,
Mark

[toc] | [prev] | [next] | [standalone]

#1373342

On Wed, Apr 6, 2016 at 3:01 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Apr 05, 2016 at 11:09:34PM +0300, Octavian Purdila wrote:
>> On Tue, Apr 5, 2016 at 9:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > * The firmware is to some extent expected to manage pinctrl today (for power
>> >   management of devices it does know about), and hence a pinctrl device is
>> >   potentially under shared management of ACPI and the OS.
>> >
>> > * The ACPI specification says nothing about this style of pinctrl management,
>> >   so it is unclear what the expectations are:
>>
>> Does it say anything at all about pinctrl management?
>
> To the best of my knowledge the ACPI spec does not explicitly mention pinctrl.
> In another reply it was mentioned that there is work in progress for pinmuxing,
> so evidently it is on the ASWG radar and within the scope of ACPI.
>
> I was trying to point out that it is _implicitly_ firmware's responsibility to
> do any pinctrl today, due to the ACPI model for power management, and the lack
> of an existing ACPI mechanisms to provide pinctrl data.
>
> In practice, firmware configures pinctrl at boot, and may modify pinctrl as
> part of the runtime power management firmware is put in charge of.
>

AFAIK the firmware only uses the pinctrl at boot to set the initial
values and the OS owns it after boot. The only interaction I know of
after boot are GPIO signaled events, but those are executed under the
control of the OS.

I don't understand the part about firmware being put in charge of
runtime power management. Do you mean that the firmware directly
accesses the pinctrl registers? Doesn't this contradict the ACPI goal
of having the OS control power management? Or do you mean accessing
pinctrl registers via _PSx and PowerResource._On/_Off?

> The lack of any statement about pinctrl would mean that there is effectively no
> reasonable limitation on what firmware might do with pinctrl, and we cannot
> assume specific behaviour from the firmware.
>

There is noting specified in the spec about other controllers, why is
pinctrl special in this regard?

> [...]
>
>> Since our focus is for open-ended configurations and for hardware that
>> it is not know to firmware we only considered the case where the pins
>> are not touched after the system boots.
>>
>> Now I wonder what are the cases were the firmware changes the pin
>> configuration after boot.
>
> Device power management and suspend/resume seem like the obvious cases.

I assume you are suggesting something like the following: we have a
device that is powered via a GPIO and associated ACPI _PS3/_PS0
methods are poking the pinctrl register to drive 0 or 1 to power on or
off the device.

If that is the case, we can easily break that today, by changing that
particular GPIO value via, e.g., sysfs.

[toc] | [prev] | [next] | [standalone]

#1373500

On Thu, Apr 07, 2016 at 03:11:53PM +0300, Octavian Purdila wrote:
> On Wed, Apr 6, 2016 at 3:01 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Apr 05, 2016 at 11:09:34PM +0300, Octavian Purdila wrote:
> >> On Tue, Apr 5, 2016 at 9:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > * The firmware is to some extent expected to manage pinctrl today (for power
> >> >   management of devices it does know about), and hence a pinctrl device is
> >> >   potentially under shared management of ACPI and the OS.
> >> >
> >> > * The ACPI specification says nothing about this style of pinctrl management,
> >> >   so it is unclear what the expectations are:
> >>
> >> Does it say anything at all about pinctrl management?
> >
> > To the best of my knowledge the ACPI spec does not explicitly mention pinctrl.
> > In another reply it was mentioned that there is work in progress for pinmuxing,
> > so evidently it is on the ASWG radar and within the scope of ACPI.
> >
> > I was trying to point out that it is _implicitly_ firmware's responsibility to
> > do any pinctrl today, due to the ACPI model for power management, and the lack
> > of an existing ACPI mechanisms to provide pinctrl data.
> >
> > In practice, firmware configures pinctrl at boot, and may modify pinctrl as
> > part of the runtime power management firmware is put in charge of.
> >
> 
> AFAIK the firmware only uses the pinctrl at boot to set the initial
> values and the OS owns it after boot. The only interaction I know of
> after boot are GPIO signaled events, but those are executed under the
> control of the OS.
> 
> I don't understand the part about firmware being put in charge of
> runtime power management. Do you mean that the firmware directly
> accesses the pinctrl registers? Doesn't this contradict the ACPI goal
> of having the OS control power management? Or do you mean accessing
> pinctrl registers via _PSx and PowerResource._On/_Off?

I mostly mean the latter. Even if the OS is in charge of _when_ those happen,
that only solves mutual exclusion over the pinctrl registers. That does not
handle expectations regarding the current _state_ of the pinctrl configuation,
or the configurations the OS/FW can permit and/or require (e.g. there's no
refcounting between OS and FW for the state of shared pins).

It may also be that pinctrl gets altered in the background (e.g. in SMM),
outside of the OS's control also, but that's probably a rare/extreme case.

> > The lack of any statement about pinctrl would mean that there is effectively no
> > reasonable limitation on what firmware might do with pinctrl, and we cannot
> > assume specific behaviour from the firmware.
> 
> There is noting specified in the spec about other controllers, why is
> pinctrl special in this regard?

Because there are clear demonstrable cases why FW would want to touch pinctrl
today, that may clash with the pinctrl model you are importing from DT (where
the OS is assumed to have complete ownership and control over pinctrl).

The ACPI model implies FW-driven pinctrl management, so if we're going to put
the OS in direct control of pinctrl, we have to make clear what expectation FW
and OS can have.

> > [...]
> >
> >> Since our focus is for open-ended configurations and for hardware that
> >> it is not know to firmware we only considered the case where the pins
> >> are not touched after the system boots.
> >>
> >> Now I wonder what are the cases were the firmware changes the pin
> >> configuration after boot.
> >
> > Device power management and suspend/resume seem like the obvious cases.
> 
> I assume you are suggesting something like the following: we have a
> device that is powered via a GPIO and associated ACPI _PS3/_PS0
> methods are poking the pinctrl register to drive 0 or 1 to power on or
> off the device.

Potentially. In that case it's not clear what the FW is expected to do, and
what the OS is expected to do.

For things like system suspend/resume or hibernate, it's not clear what state
the FW is expected to save/restore, and what state might arbitrarily change.

> If that is the case, we can easily break that today, by changing that
> particular GPIO value via, e.g., sysfs.

Sure, but that is not part of the usual, automatic management of the system.

There are plenty of ways a user with sufficient privilege can bring easily down
a system. They are irrelevant.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1373789

On Wednesday, April 06, 2016 11:39:27 AM Mark Rutland wrote:
> On Thu, Apr 07, 2016 at 03:11:53PM +0300, Octavian Purdila wrote:
> > On Wed, Apr 6, 2016 at 3:01 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Tue, Apr 05, 2016 at 11:09:34PM +0300, Octavian Purdila wrote:
> > >> On Tue, Apr 5, 2016 at 9:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > >> > * The firmware is to some extent expected to manage pinctrl today (for power
> > >> >   management of devices it does know about), and hence a pinctrl device is
> > >> >   potentially under shared management of ACPI and the OS.
> > >> >
> > >> > * The ACPI specification says nothing about this style of pinctrl management,
> > >> >   so it is unclear what the expectations are:
> > >>
> > >> Does it say anything at all about pinctrl management?
> > >
> > > To the best of my knowledge the ACPI spec does not explicitly mention pinctrl.
> > > In another reply it was mentioned that there is work in progress for pinmuxing,
> > > so evidently it is on the ASWG radar and within the scope of ACPI.
> > >
> > > I was trying to point out that it is _implicitly_ firmware's responsibility to
> > > do any pinctrl today, due to the ACPI model for power management, and the lack
> > > of an existing ACPI mechanisms to provide pinctrl data.
> > >
> > > In practice, firmware configures pinctrl at boot, and may modify pinctrl as
> > > part of the runtime power management firmware is put in charge of.
> > >
> > 
> > AFAIK the firmware only uses the pinctrl at boot to set the initial
> > values and the OS owns it after boot. The only interaction I know of
> > after boot are GPIO signaled events, but those are executed under the
> > control of the OS.
> > 
> > I don't understand the part about firmware being put in charge of
> > runtime power management. Do you mean that the firmware directly
> > accesses the pinctrl registers? Doesn't this contradict the ACPI goal
> > of having the OS control power management? Or do you mean accessing
> > pinctrl registers via _PSx and PowerResource._On/_Off?
> 
> I mostly mean the latter. Even if the OS is in charge of _when_ those happen,
> that only solves mutual exclusion over the pinctrl registers. That does not
> handle expectations regarding the current _state_ of the pinctrl configuation,
> or the configurations the OS/FW can permit and/or require (e.g. there's no
> refcounting between OS and FW for the state of shared pins).
> 
> It may also be that pinctrl gets altered in the background (e.g. in SMM),
> outside of the OS's control also, but that's probably a rare/extreme case.
> 
> > > The lack of any statement about pinctrl would mean that there is effectively no
> > > reasonable limitation on what firmware might do with pinctrl, and we cannot
> > > assume specific behaviour from the firmware.
> > 
> > There is noting specified in the spec about other controllers, why is
> > pinctrl special in this regard?
> 
> Because there are clear demonstrable cases why FW would want to touch pinctrl
> today, that may clash with the pinctrl model you are importing from DT (where
> the OS is assumed to have complete ownership and control over pinctrl).
> 
> The ACPI model implies FW-driven pinctrl management, so if we're going to put
> the OS in direct control of pinctrl, we have to make clear what expectation FW
> and OS can have.

Well, orthodox ACPI people from the Windows camp would definitely agree with you,
but I'm not one of them, so let me play a devil's advocate for a while. :-)

IMO, exposing anything in the ACPI tables without explicitly saying "but you
should not touch that", eg by defining an operation region covering it or similar,
is equivalent to giving the OS a license to play with that thing as it wishes.

Therefore I would regard exposing pin control information in cases when the
OS is not allowed to use it to its fit as a firmware bug.

Now, the question in this patch series is how to expose things and not when to
do that.  It adds support for a specific method of exposing information to the
OS, but should it be concerned about the possible consequences of inappropriate
use of that method by firmware?

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1376743

[Multipart message — attachments visible in raw view] — view raw

On Thu, Apr 07, 2016 at 11:24:00PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, April 06, 2016 11:39:27 AM Mark Rutland wrote:

> > The ACPI model implies FW-driven pinctrl management, so if we're going to put
> > the OS in direct control of pinctrl, we have to make clear what expectation FW
> > and OS can have.

> Well, orthodox ACPI people from the Windows camp would definitely agree with you,
> but I'm not one of them, so let me play a devil's advocate for a while. :-)

> IMO, exposing anything in the ACPI tables without explicitly saying "but you
> should not touch that", eg by defining an operation region covering it or similar,
> is equivalent to giving the OS a license to play with that thing as it wishes.

> Therefore I would regard exposing pin control information in cases when the
> OS is not allowed to use it to its fit as a firmware bug.

This is something where it seems like it would be very easy for people
to end up relying on current OS behaviour even if that only happened
through accident rather than design.  You might assume that such
behaviour is a bug but that may not be obvious to the firmware author if
the OS behaviour makes sense to them.

There may also be some expectation on the part of firmware that the OS
will do some management of the pins even for devices that it's not
actively working with (put them in an idle mode for example).

> Now, the question in this patch series is how to expose things and not when to
> do that.  It adds support for a specific method of exposing information to the
> OS, but should it be concerned about the possible consequences of inappropriate
> use of that method by firmware?

The other issue here is that if (as Octavian mentioned) there are
ongoing discussions within ASWG on producing an actual spec for this it
doesn't seem great that we'd just go and do something completely
different rather than trying to make sure that the spec works well.  Or
if there isn't any spec work then writing one there for other OSs to
pick up if they like.  This seems like it'd make us much better
community players.

[toc] | [prev] | [next] | [standalone]

#1377510

On Tue, Apr 12, 2016 at 2:15 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Apr 07, 2016 at 11:24:00PM +0200, Rafael J. Wysocki wrote:
>> On Wednesday, April 06, 2016 11:39:27 AM Mark Rutland wrote:
>
>> > The ACPI model implies FW-driven pinctrl management, so if we're going to put
>> > the OS in direct control of pinctrl, we have to make clear what expectation FW
>> > and OS can have.
>
>> Well, orthodox ACPI people from the Windows camp would definitely agree with you,
>> but I'm not one of them, so let me play a devil's advocate for a while. :-)
>
>> IMO, exposing anything in the ACPI tables without explicitly saying "but you
>> should not touch that", eg by defining an operation region covering it or similar,
>> is equivalent to giving the OS a license to play with that thing as it wishes.
>
>> Therefore I would regard exposing pin control information in cases when the
>> OS is not allowed to use it to its fit as a firmware bug.
>
> This is something where it seems like it would be very easy for people
> to end up relying on current OS behaviour even if that only happened
> through accident rather than design.  You might assume that such
> behaviour is a bug but that may not be obvious to the firmware author if
> the OS behaviour makes sense to them.

If the OS started to support that interface, it would need to
guarantee consistent behavior with respect to it going forward.

It would be an interface for interacting with firmware, so it is not
much different from an interface to user space as far as the stability
rules go.  Which may be a reason for refusing to support it in the
first place if the OS is not willing to make such a guarantee.

> There may also be some expectation on the part of firmware that the OS
> will do some management of the pins even for devices that it's not
> actively working with (put them in an idle mode for example).
>
>> Now, the question in this patch series is how to expose things and not when to
>> do that.  It adds support for a specific method of exposing information to the
>> OS, but should it be concerned about the possible consequences of inappropriate
>> use of that method by firmware?
>
> The other issue here is that if (as Octavian mentioned) there are
> ongoing discussions within ASWG on producing an actual spec for this it
> doesn't seem great that we'd just go and do something completely
> different rather than trying to make sure that the spec works well.  Or
> if there isn't any spec work then writing one there for other OSs to
> pick up if they like.  This seems like it'd make us much better
> community players.

Agreed.  I actually asked Octavian to look at the other proposal and
see if/how that could be used to address the use case in question
and/or possibly extended to cover it.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1372087

On 04/05/2016 02:56 AM, Charles Garcia-Tobin wrote:
> 
> 
> On 04/04/2016 23:52, "Mark Rutland" <mark.rutland@arm.com> wrote:
> 
>> Hi,
>>
>> On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
>>> This is a proposal for adding ACPI support for pin controller
>>> configuration.
>>>
>>> It has been developed to enable the MinnowBoard and IoT community
>>> by providing an easy way to specify pin multiplexing and
>>> pin configuration.
>>>
>>> This proposal is based on using _DSD properties to specify device
>>> states and configuration nodes and it follows closely the device
>>> tree model. Device states are defined using the Device Properties
>>> format and the configuration nodes are defined using the
>>> Hierarchical Properties Extension format. The generic properties
>>> for the configuration nodes are the same as the ones for device
>>> tree, while pincontroller drivers can also define custom ones.
>>
>>From a look of the Documentation addition, and of the current uses of
>> pinctrl-names in device tree bindings, one reason for requiring multiple
>> pinctrl states is power management. Given that, I'm somewhat concerned by
>> this,
>> as it goes against the usual ACPI model of abstracting this sort of thing
>> behind power control methods.
>>
>> To the best of my knowledge, that goes against the ASWG's expectations on
>> how
>> _DSD will be used (per [1]). Charles, please correct me if that document
>> is no
>> longer representative.
> 
> It is though latest version was posted by Rafael a bit later:
> https://lists.acpica.org/pipermail/dsd/2015-December/000027.html
> 
> 
> In addition the core rules requiring that existing ACPI paradigms are not
> subverted through DSD (basically the concern you express) are also
> documented in the main DSD documentation itself:
> http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UU
> ID.pdf (section 2.3)

I have to echo Mark's concern: wholesale importation of portions of current
DT bindings simply because it's expedient is one of the things I've been hoping
to avoid.  These patches seem to be just that.

And while the latest version mentioned above describes a bit of a review
process to handle this case, I don't recall the kernel community at large
agreeing to it, nor to it having been implemented.  If I missed that part,
my apologies; please let me know where it was decided.  If I haven't, then
perhaps this needs to be the first test case of that process.

-- 
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------

[toc] | [prev] | [next] | [standalone]

#1372393

On Tue, Apr 05, 2016 at 06:00:31PM -0600, Al Stone wrote:
> On 04/05/2016 02:56 AM, Charles Garcia-Tobin wrote:
> > 
> > 
> > On 04/04/2016 23:52, "Mark Rutland" <mark.rutland@arm.com> wrote:
> > 
> >> Hi,
> >>
> >> On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
> >>> This is a proposal for adding ACPI support for pin controller
> >>> configuration.
> >>>
> >>> It has been developed to enable the MinnowBoard and IoT community
> >>> by providing an easy way to specify pin multiplexing and
> >>> pin configuration.
> >>>
> >>> This proposal is based on using _DSD properties to specify device
> >>> states and configuration nodes and it follows closely the device
> >>> tree model. Device states are defined using the Device Properties
> >>> format and the configuration nodes are defined using the
> >>> Hierarchical Properties Extension format. The generic properties
> >>> for the configuration nodes are the same as the ones for device
> >>> tree, while pincontroller drivers can also define custom ones.
> >>
> >>From a look of the Documentation addition, and of the current uses of
> >> pinctrl-names in device tree bindings, one reason for requiring multiple
> >> pinctrl states is power management. Given that, I'm somewhat concerned by
> >> this,
> >> as it goes against the usual ACPI model of abstracting this sort of thing
> >> behind power control methods.
> >>
> >> To the best of my knowledge, that goes against the ASWG's expectations on
> >> how
> >> _DSD will be used (per [1]). Charles, please correct me if that document
> >> is no
> >> longer representative.
> > 
> > It is though latest version was posted by Rafael a bit later:
> > https://lists.acpica.org/pipermail/dsd/2015-December/000027.html
> > 
> > 
> > In addition the core rules requiring that existing ACPI paradigms are not
> > subverted through DSD (basically the concern you express) are also
> > documented in the main DSD documentation itself:
> > http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UU
> > ID.pdf (section 2.3)
> 
> I have to echo Mark's concern: wholesale importation of portions of current
> DT bindings simply because it's expedient is one of the things I've been hoping
> to avoid.  These patches seem to be just that.
> 
> And while the latest version mentioned above describes a bit of a review
> process to handle this case, I don't recall the kernel community at large
> agreeing to it, nor to it having been implemented.  If I missed that part,
> my apologies; please let me know where it was decided.  If I haven't, then
> perhaps this needs to be the first test case of that process.
> 

My concern over this is similar to Mark/Als this looks like a quick hack
to "solve" an issue that has been worked on in Linux for at least 10
years now. And thats how to describe a non descoverable card that is
plugged into random busses on a SoC.

The issue I see with a quick hack of DT pinctrl into _DSD is that this
does not take into account the power model of ACPI. As far as I can see
the core code has no manner to tell a pin/function allocated through
_DSD actually has effects on the power of other devices. So the core
could end up powering down devices when it should not. This is very
relevent to your intended use of IoT devices where power control is key.

Why has there been no attempt in ASWG to make these sort of features a
1st class citizen of ACPI so they can interact correctly with the other
features?

Thanks

Graeme

[toc] | [prev] | [next] | [standalone]

#1373417

On Wed, Apr 6, 2016 at 1:49 PM, Graeme Gregory <gg@slimlogic.co.uk> wrote:

>>
>> I have to echo Mark's concern: wholesale importation of portions of current
>> DT bindings simply because it's expedient is one of the things I've been hoping
>> to avoid.  These patches seem to be just that.
>>
>> And while the latest version mentioned above describes a bit of a review
>> process to handle this case, I don't recall the kernel community at large
>> agreeing to it, nor to it having been implemented.  If I missed that part,
>> my apologies; please let me know where it was decided.  If I haven't, then
>> perhaps this needs to be the first test case of that process.
>>
>
> My concern over this is similar to Mark/Als this looks like a quick hack
> to "solve" an issue that has been worked on in Linux for at least 10
> years now. And thats how to describe a non descoverable card that is
> plugged into random busses on a SoC.
>
> The issue I see with a quick hack of DT pinctrl into _DSD is that this
> does not take into account the power model of ACPI. As far as I can see
> the core code has no manner to tell a pin/function allocated through
> _DSD actually has effects on the power of other devices. So the core
> could end up powering down devices when it should not. This is very
> relevent to your intended use of IoT devices where power control is key.
>

It is not clear to me how this could happen - core powering down
devices when it should not, unless pins are shared.  And if I am not
missing something, pinctrl is designed to handle this case by only by
switching to idle/suspend/default states from the controller drivers
in the suspend/resume routine. ACPI _Sx calls also happens at the same
time so there does not seem to be a contention point.

Perhaps I am missing something and an example of such issue would make
it more clear.

And, as our primary use case is to set the initial muxing, we can
easily remove the suspend/resume functionality if this turns out to be
the issue.

> Why has there been no attempt in ASWG to make these sort of features a
> 1st class citizen of ACPI so they can interact correctly with the other
> features?
>

IMO having an ASWG pinctrl specification and using _DSD for Linux are
orthogonal approaches. The latter is very specific to Linux and we
want it so that we can support the Linux pinctrl model as best as
possible and I doubt it is a common denominator for all OSes that ACPI
supports.

[toc] | [prev] | [next] | [standalone]

#1373625

On Thu, Apr 7, 2016 at 4:17 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Wed, Apr 6, 2016 at 1:49 PM, Graeme Gregory <gg@slimlogic.co.uk> wrote:

>> Why has there been no attempt in ASWG to make these sort of features a
>> 1st class citizen of ACPI so they can interact correctly with the other
>> features?
>
> IMO having an ASWG pinctrl specification and using _DSD for Linux are
> orthogonal approaches. The latter is very specific to Linux and we
> want it so that we can support the Linux pinctrl model as best as
> possible and I doubt it is a common denominator for all OSes that ACPI
> supports.

Sob, I tried so hard to implement pin control referring to generic
concepts that everyone should be able to reuse. E.g. stressing SI
units to be used all over the place, neutral scientific terms for all
config options and what not.

The multiplexing with groups and functions is a simple (aehm,
OK, super-complicated) matter of mapping disjunct sets onto each
other, so that comes from logic.

Not even the GPIO ranges that map GPIOs to pin ranges are very
Linux-specific: it is a property of the hardware that sometimes
GPIO is "behind" the pin, in a distinct hardware unit. (See pinctrl.txt
for the GPIO pitfalls.)

If I was to rewrite pin control and GPIO from scratch I would make it
one subsystem instead of two. This is more of an architectural
choice. GPIO would still be a stand-alone part of it.

Yours,
Linus Walleij

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web