From: Jeremy Linton <hidden> Date: 2015-08-12 22:06:25
This set of patches enables the front Ethernet port on the
ARM Juno development platform when used with an ACPI enabled kernel.
These patches covert the of_property* calls in the driver to the
DT/ACPI agnostic device_property* calls, and add the arm hardware
id to the acpi_match_table.
To support the above changes I copied a couple routines from
of_net into the properties.c file, and modified them to
be ACPI/DT agnostic. I'm not 100% sure this is the correct location
for these functions. But I think they are required to avoid having
a dozen different implementations scattered across assorted Ethernet
adapters that are being enabled to use ACPI properties.
Jeremy Linton (2):
Add a matching set of device_ functions for determining mac/phy
Convert smsc911x to use ACPI as well as DT
drivers/base/property.c | 73 ++++++++++++++++++++++++++++++++++++
drivers/net/ethernet/smsc/smsc911x.c | 48 +++++++++++-------------
include/linux/property.h | 4 ++
3 files changed, 99 insertions(+), 26 deletions(-)
--
2.4.3
From: Jeremy Linton <hidden> Date: 2015-08-12 22:06:26
OF has some helper functions for parsing MAC and PHY settings.
In cases where the platform is providing this information rather
than the device itself, there needs to be similar functions for ACPI.
These functions are slightly modified versions of the ones in
of_net which can use information provided via DT or ACPI.
Signed-off-by: Jeremy Linton <redacted>
---
drivers/base/property.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/property.h | 4 +++
2 files changed, 77 insertions(+)
From: Jeremy Linton <hidden> Date: 2015-08-12 22:06:27
Add ACPI bindings for the smsc911x driver. Convert the DT specific calls
to nonspecific device* calls, This allows the driver to work
with both ACPI and DT configurations. Ethernet should now work when using
ACPI on ARM Juno.
Signed-off-by: Jeremy Linton <redacted>
---
drivers/net/ethernet/smsc/smsc911x.c | 48 +++++++++++++++++-------------------
1 file changed, 22 insertions(+), 26 deletions(-)
OF has some helper functions for parsing MAC and PHY settings.
In cases where the platform is providing this information rather
than the device itself, there needs to be similar functions for ACPI.
These functions are slightly modified versions of the ones in
of_net which can use information provided via DT or ACPI.
Signed-off-by: Jeremy Linton <redacted>
---
[snip]
+
+static void *device_get_mac_addr(struct device *dev,
+ const char *name, char *addr,
+ int alen)
+{
+ int ret = device_property_read_u8_array(dev, name, addr, alen);
+
+ if (ret == 0 && is_valid_ether_addr(addr))
+ return addr;
+ return NULL;
The DT counterpart has an additional check on the properly length to be
ETH_ALEN, you might want to have such check here for consistency and
correctness.
Other than that, this looks good to me.
--
Florian
On Wed, Aug 12, 2015 at 05:06:27PM -0500, Jeremy Linton wrote:
Add ACPI bindings for the smsc911x driver. Convert the DT specific calls
to nonspecific device* calls, This allows the driver to work
with both ACPI and DT configurations. Ethernet should now work when using
ACPI on ARM Juno.
Signed-off-by: Jeremy Linton <redacted>
From: Lorenzo Pieralisi <hidden> Date: 2015-08-13 09:01:17
On Thu, Aug 13, 2015 at 09:27:59AM +0100, Graeme Gregory wrote:
On Wed, Aug 12, 2015 at 05:06:27PM -0500, Jeremy Linton wrote:
quoted
Add ACPI bindings for the smsc911x driver. Convert the DT specific calls
to nonspecific device* calls, This allows the driver to work
with both ACPI and DT configurations. Ethernet should now work when using
ACPI on ARM Juno.
What about _DSD device specific properties (eg "smsc,save-mac-address") ?
Are we taking 1:1 translation between DT and ACPI for granted ?
I thought some process must be put in place to define the corresponding
bindings in ACPI before starting this mechanical translation or maybe
I missed something, I would like to understand.
How does the device specific _DSD definitions work in ACPI world ? Where
are they published ? How will we translate those to DT bindings if there
is need ?
Lorenzo
On Thu, Aug 13, 2015 at 10:01:17AM +0100, Lorenzo Pieralisi wrote:
On Thu, Aug 13, 2015 at 09:27:59AM +0100, Graeme Gregory wrote:
quoted
On Wed, Aug 12, 2015 at 05:06:27PM -0500, Jeremy Linton wrote:
quoted
Add ACPI bindings for the smsc911x driver. Convert the DT specific calls
to nonspecific device* calls, This allows the driver to work
with both ACPI and DT configurations. Ethernet should now work when using
ACPI on ARM Juno.
What about _DSD device specific properties (eg "smsc,save-mac-address") ?
Are we taking 1:1 translation between DT and ACPI for granted ?
I thought some process must be put in place to define the corresponding
bindings in ACPI before starting this mechanical translation or maybe
I missed something, I would like to understand.
How does the device specific _DSD definitions work in ACPI world ? Where
are they published ? How will we translate those to DT bindings if there
is need ?
This I do not know as that discussion is still ongoing. But those
options are currently marked as optional so driver should not fail if
they are not present in ACPI.
Graeme
From: Lorenzo Pieralisi <hidden> Date: 2015-08-13 10:30:08
On Thu, Aug 13, 2015 at 10:38:38AM +0100, Graeme Gregory wrote:
On Thu, Aug 13, 2015 at 10:01:17AM +0100, Lorenzo Pieralisi wrote:
quoted
On Thu, Aug 13, 2015 at 09:27:59AM +0100, Graeme Gregory wrote:
quoted
On Wed, Aug 12, 2015 at 05:06:27PM -0500, Jeremy Linton wrote:
quoted
Add ACPI bindings for the smsc911x driver. Convert the DT specific calls
to nonspecific device* calls, This allows the driver to work
with both ACPI and DT configurations. Ethernet should now work when using
ACPI on ARM Juno.
What about _DSD device specific properties (eg "smsc,save-mac-address") ?
Are we taking 1:1 translation between DT and ACPI for granted ?
I thought some process must be put in place to define the corresponding
bindings in ACPI before starting this mechanical translation or maybe
I missed something, I would like to understand.
How does the device specific _DSD definitions work in ACPI world ? Where
are they published ? How will we translate those to DT bindings if there
is need ?
This I do not know as that discussion is still ongoing. But those
options are currently marked as optional so driver should not fail if
they are not present in ACPI.
Well yes, but if they are present this patch uses them when booting
with ACPI and that's what I am arguing about, because they are
undocumented.
This discussion should be brought to completion, the _DSD usage and
bindings sharing between DT and ACPI well defined before we can
enable drivers to rely on properties that in ACPI world have no
binding definition at all, if we go by this route we might end
up having drivers reusing DT properties that have no reason whatsoever
to exist in ACPI world.
So, to make this clear, the _DSD usage must be documented and there
has to be a way to define the related bindings in ACPI world so that
driver code can be reviewed accordingly.
Lorenzo
Hi Jeremy,
On 12/08/15 23:06, Jeremy Linton wrote:
[...]
+static void *device_get_mac_addr(struct device *dev,
+ const char *name, char *addr,
+ int alen)
+{
+ int ret = device_property_read_u8_array(dev, name, addr, alen);
+
+ if (ret == 0 && is_valid_ether_addr(addr))
+ return addr;
+ return NULL;
+}
Not sure I understand the logic here - "return the same thing we were
given if we updated it, or null if we didn't". It's only indicating
success/failure (the caller can perfectly well cast its own buffer to a
void * if it needs to), so why wouldn't you just return a normal int
error code?
+/**
+ * Search the device tree for the best MAC address to use. 'mac-address' is
+ * checked first, because that is supposed to contain to "most recent" MAC
+ * address. If that isn't set, then 'local-mac-address' is checked next,
+ * because that is the default address. If that isn't set, then the obsolete
+ * 'address' is checked, just in case we're using an old device tree.
+ *
+ * Note that the 'address' property is supposed to contain a virtual address of
+ * the register set, but some DTS files have redefined that property to be the
+ * MAC address.
+ *
+ * All-zero MAC addresses are rejected, because those could be properties that
+ * exist in the device tree, but were not set by U-Boot. For example, the
+ * DTS could define 'mac-address' and 'local-mac-address', with zero MAC
+ * addresses. Some older U-Boots only initialized 'local-mac-address'. In
+ * this case, the real MAC is in 'local-mac-address', and 'mac-address' exists
+ * but is all zeros.
+*/
+void *device_get_mac_address(struct device *dev, char *addr, int alen)
+{
+ addr = device_get_mac_addr(dev, "mac-address", addr, alen);
+ if (addr)
+ return addr;
+
+ addr = device_get_mac_addr(dev, "local-mac-address", addr, alen);
+ if (addr)
+ return addr;
+
+ return device_get_mac_addr(dev, "address", addr, alen);
+}
+EXPORT_SYMBOL(device_get_mac_address);
Same here, it's not at all apparent why this should return a void *
instead of an int (or even possibly bool). of_get_mac_address is giving
its caller back a _new_ pointer they didn't know about before; this isn't.
Robin.
From: Jeremy Linton <hidden> Date: 2015-08-13 14:24:23
Hello Robin,
On 08/13/2015 06:57 AM, Robin Murphy wrote:
quoted
+static void *device_get_mac_addr(struct device *dev,
+ const char *name, char *addr,
+ int alen)
+{
+ int ret = device_property_read_u8_array(dev, name, addr, alen);
+
+ if (ret == 0 && is_valid_ether_addr(addr))
+ return addr;
+ return NULL;
+}
Not sure I understand the logic here - "return the same thing we were
given if we updated it, or null if we didn't". It's only indicating
success/failure (the caller can perfectly well cast its own buffer to a
void * if it needs to), so why wouldn't you just return a normal int
error code?
No particular reason, other than initially I was trying to keep the
function as similar as possible to the one in of_net. AKA copy paste
job. I can convert the return types, but I was trying for a simple
function rename. That way the users of the of version could be converted
with relative ease, and the drivers which invented their own version of
these functions could be changed to use this instead. Of course, that
plan took a blow, when I added the addr/alen parameters.
Same thing applies for the other function.
From: Jeremy Linton <redacted>
Date: Wed, 12 Aug 2015 17:06:25 -0500
This set of patches enables the front Ethernet port on the
ARM Juno development platform when used with an ACPI enabled kernel.
These patches covert the of_property* calls in the driver to the
DT/ACPI agnostic device_property* calls, and add the arm hardware
id to the acpi_match_table.
To support the above changes I copied a couple routines from
of_net into the properties.c file, and modified them to
be ACPI/DT agnostic. I'm not 100% sure this is the correct location
for these functions. But I think they are required to avoid having
a dozen different implementations scattered across assorted Ethernet
adapters that are being enabled to use ACPI properties.
I realize that there are still some rinkles to work out, but I applied
this patch series as-is to net-next, and any fixups should be
submitted as followups.
Thanks.
From: Jeremy Linton <hidden> Date: 2015-08-14 15:55:53
On 08/12/2015 05:13 PM, Florian Fainelli wrote:
On 12/08/15 15:06, Jeremy Linton wrote:
quoted
+
+static void *device_get_mac_addr(struct device *dev,
+ const char *name, char *addr,
+ int alen)
+{
+ int ret = device_property_read_u8_array(dev, name, addr, alen);
+
+ if (ret == 0 && is_valid_ether_addr(addr))
+ return addr;
+ return NULL;
The DT counterpart has an additional check on the properly length to be
ETH_ALEN, you might want to have such check here for consistency and
correctness.
From: Marc Zyngier <hidden> Date: 2015-09-09 16:10:33
Jeremy,
On 12/08/15 23:06, Jeremy Linton wrote:
Add ACPI bindings for the smsc911x driver. Convert the DT specific calls
to nonspecific device* calls, This allows the driver to work
with both ACPI and DT configurations. Ethernet should now work when using
ACPI on ARM Juno.
Signed-off-by: Jeremy Linton <redacted>
---
drivers/net/ethernet/smsc/smsc911x.c | 48 +++++++++++++++++-------------------
1 file changed, 22 insertions(+), 26 deletions(-)
From: Jeremy Linton <hidden> Date: 2015-09-23 17:22:49
Marc,
|FWIW, mainline booting with this patch on Juno r1 with ACPI enabled dies a
|horrible death:
Sorry about the delay, I didn't see this message.
<trimmed>
|How did you get this to work? Firmware release?
Yes, it's a firmware problem with the ACPI tables. It is likely you need _DSD changes for the SMSC. If your trying to run juno with ACPI there are a few other ACPI table changes you will need beyond what was in the last linaro release. I wanted to just provide a direct link to the correct firmware in this response, but it seems the required version is sort of up in the air at the moment. Be assured I will really push on this next week, and will provide a follow up here.
Originally, the lack of DSD wasn't a problem, but as this patch evolved the fact that it consolidates a number of configuration code paths balancing that got tricky, and ACPI machines with "bad" tables were the victims (rather than breaking something I can't control).
In the meantime, if this is blocking anyone I can provide instructions on how to update the required ACPI tables.
Thanks,
-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590
ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782
From: Marc Zyngier <hidden> Date: 2015-09-23 17:46:24
On Wed, 23 Sep 2015 17:22:49 +0000
Jeremy Linton [off-list ref] wrote:
Hi Jeremy,
Marc,
|FWIW, mainline booting with this patch on Juno r1 with ACPI enabled dies a
|horrible death:
Sorry about the delay, I didn't see this message.
<trimmed>
|How did you get this to work? Firmware release?
Yes, it's a firmware problem with the ACPI tables. It is likely you
need _DSD changes for the SMSC. If your trying to run juno with ACPI
there are a few other ACPI table changes you will need beyond what
was in the last linaro release. I wanted to just provide a direct
link to the correct firmware in this response, but it seems the
required version is sort of up in the air at the moment. Be assured I
will really push on this next week, and will provide a follow up here.
Good. Please keep me posted on this. I understand this is still a work
in progress, and it would be good to document what is the expected
baseline for this to work correctly.
Originally, the lack of DSD wasn't a problem, but as this patch
evolved the fact that it consolidates a number of configuration code
paths balancing that got tricky, and ACPI machines with "bad" tables
were the victims (rather than breaking something I can't control).
In the meantime, if this is blocking anyone I can provide
instructions on how to update the required ACPI tables.
Thanks,
M.
--
Jazz is not dead. It just smells funny.
Marc,
|FWIW, mainline booting with this patch on Juno r1 with ACPI enabled
dies a |horrible death:
Sorry about the delay, I didn't see this message.
<trimmed>
|How did you get this to work? Firmware release?
Yes, it's a firmware problem with the ACPI tables. It is likely you
need _DSD changes for the SMSC. If your trying to run juno with ACPI
there are a few other ACPI table changes you will need beyond what
was in the last linaro release. I wanted to just provide a direct
link to the correct firmware in this response, but it seems the
required version is sort of up in the air at the moment. Be assured I
will really push on this next week, and will provide a follow up
here.
Originally, the lack of DSD wasn't a problem, but as this patch
evolved the fact that it consolidates a number of configuration code
paths balancing that got tricky, and ACPI machines with "bad" tables
were the victims (rather than breaking something I can't control).
Even now absence of _DSD *should* continue to be just fine. We can
bailout without initializing the device. Pulling the entire kernel down
just because of an incomplete device entry make no sense IMO.
In the meantime, if this is blocking anyone I can provide
instructions on how to update the required ACPI tables.
I found that the driver has issues even with DT missing some of the
optional properties. We need to fix it but I don't have much knowledge
of the driver and also not sure how these properties are defined in
ACPI/DSD world i.e. required vs optional.
Regards,
Sudeep
Hm, surely you shouldn't need this part? If the ACPI device has a HID
of PRP0001, and an appropriate "compatible" property, then it ought to
be loaded anyway. If that doesn't work, we should fix that.
--
dwmw2
Hm, surely you shouldn't need this part? If the ACPI device has a HID
of PRP0001, and an appropriate "compatible" property, then it ought to
be loaded anyway. If that doesn't work, we should fix that.
But if the device ID is assigned already, why would it hurt to actually
use it?
Thanks,
Rafael
On Wed, 2015-09-23 at 22:51 +0200, Rafael J. Wysocki wrote:
But if the device ID is assigned already, why would it hurt to
actually use it?
It doesn't hurt at all, as long as we understand that there was no need
to assign it a device ID at all, at least for Linux's benefit. And as
long as it doesn't set a precedent that makes other people think they
need to do so.
--
dwmw2
On Wed, 2015-09-23 at 22:51 +0200, Rafael J. Wysocki wrote:
quoted
But if the device ID is assigned already, why would it hurt to
actually use it?
It doesn't hurt at all, as long as we understand that there was no need
to assign it a device ID at all, at least for Linux's benefit. And as
long as it doesn't set a precedent that makes other people think they
need to do so.
It really depends on the people who writing the ASL code (DSDT),
I'm not sure if Windows will use _DSD and how to use it, but
firmware guys may just use the device ID to make the firmware
to be usable both by Linux and Windows, and that's reasonable
I think.
Thanks
Hanjun
On Wed, 2015-09-23 at 16:56 -0700, Hanjun Guo wrote:
It really depends on the people who writing the ASL code (DSDT),
I'm not sure if Windows will use _DSD and how to use it, but
firmware guys may just use the device ID to make the firmware
to be usable both by Linux and Windows, and that's reasonable
I think.
And the ideal way to do that is to add a _CID of "PRP0001" and a
compatible string, and then Linux doesn't need to care about the
special new HID that you've added purely to pander to the limitations
of Windows.
That way, once drivers are converted from the legacy of-property APIs
to the new device-property APIs, there should be *nothing* required to
make device drivers work under ACPI. That was kind of the point.
If people want to add HIDs, fine. But let's not go down a path which
*requires* modifying drivers.
(I'm planning to try to learn Coccinelle and do a bombing run convertin
to the new API some time soon, FWIW).
--
dwmw2
On Wed, Sep 23, 2015 at 06:57:03PM +0100, Sudeep Holla wrote:
On 23/09/15 18:22, Jeremy Linton wrote:
quoted
|FWIW, mainline booting with this patch on Juno r1 with ACPI enabled
dies a |horrible death:
Sorry about the delay, I didn't see this message.
<trimmed>
|How did you get this to work? Firmware release?
Yes, it's a firmware problem with the ACPI tables. It is likely you
need _DSD changes for the SMSC. If your trying to run juno with ACPI
there are a few other ACPI table changes you will need beyond what
was in the last linaro release. I wanted to just provide a direct
link to the correct firmware in this response, but it seems the
required version is sort of up in the air at the moment. Be assured I
will really push on this next week, and will provide a follow up
here.
Originally, the lack of DSD wasn't a problem, but as this patch
evolved the fact that it consolidates a number of configuration code
paths balancing that got tricky, and ACPI machines with "bad" tables
were the victims (rather than breaking something I can't control).
Even now absence of _DSD *should* continue to be just fine. We can
bailout without initializing the device. Pulling the entire kernel down
just because of an incomplete device entry make no sense IMO.
I agree. IMO, that's a regression since a working kernel on Juno (albeit
without networking) now dies unless you also upgrade the firmware.
--
Catalin
Hi Dave,
On Thu, Sep 24, 2015 at 09:16:19AM +0100, David Woodhouse wrote:
On Wed, 2015-09-23 at 16:56 -0700, Hanjun Guo wrote:
quoted
It really depends on the people who writing the ASL code (DSDT),
I'm not sure if Windows will use _DSD and how to use it, but
firmware guys may just use the device ID to make the firmware
to be usable both by Linux and Windows, and that's reasonable
I think.
And the ideal way to do that is to add a _CID of "PRP0001" and a
compatible string, and then Linux doesn't need to care about the
special new HID that you've added purely to pander to the limitations
of Windows.
PRP0001 could make sense from a Linux driver code reuse perspective. It
probably also makes sense for the x86 embedded world where there is ACPI
legacy. On embedded ARM we have DT already, so we don't need to solve
any ACPI issues.
However, for the ARM server ecosystem (both hardware and software), IMO,
PRP0001 will have long term negative implications. My view of the ARM
server (incipient) world is that we have two main categories of SoC
vendors targeting ACPI:
1. Vendors that have been in the server business for a long time and are
looking into using ARM processors. They usually have experience with
designing server hardware, ACPI firmware and targeting multiple OSes.
2. Vendors new to the server world (but not necessarily new to ARM) that
would like to extend their SoC family with server products. They
may not have prior experience with ACPI but are targeting it because
they've probably been told so.
What I'm worried about is the second category. We try hard to make such
vendors follow both software and hardware standards (like ARM's SBSA and
even ACPI, I don't have a problem if used properly). They really need to
get into the habit of designing their hardware and firmware without
thinking that they can always apply a Linux patch and recompile the
kernel. In the server world, the kernel (image) is no longer owned by
the SoC vendor (as it is the norm in mobile space) but by a different
entity (OS vendor) which may not patch/upgrade the kernel every time
(for example) a clock routing is changed on some SoC.
So what we _do_ want on ARM is tight control of the _DSD properties that
are going to be used in ACPI tables, avoid pitfalls like OS-specific
names ("linux-trigger" is still listed as an example on uefi.org) or
defining more than the minimum necessary. In this last category we want
to avoid properties like clocks, voltage regulators; these should be
handled in an ACPI-specific way (AML, device power states) or just fully
initialised by the boot code/firmware when run-time changes not
required.
PRP0001 opens the door to any DT-like properties in ACPI and, knowing
how the ARM ecosystem works, I'm pretty sure we'll soon lose control (if
we haven't already; not all the developments are done in the open). I'm
also not a fan of different firmware paths based on which OS is running,
so I would rather have device _HID and _DSD properties where absolutely
necessary.
(I'm planning to try to learn Coccinelle and do a bombing run convertin
to the new API some time soon, FWIW).
I think such patch would address an issue with ACPI in the x86 embedded
land. But I'd very much like to make PRP0001 parsing depend on
!CONFIG_ARM64. What we first need to solve in the ARM (server) land is
firmware and hardware standardisation rather than working around it.
The pro ARM ACPI camp has been very vocal against DT in the server
space. I'd like to seem them as vocal about PRP0001, unless they find it
acceptable (and should apologise for bashing DT ;)).
Thanks.
--
Catalin
Hi Catalin,
I understand your concerns, but I'm still not convinced of your
conclusion.
On Thu, 2015-09-24 at 11:31 +0100, Catalin Marinas wrote:
PRP0001 opens the door to any DT-like properties in ACPI and, knowing
how the ARM ecosystem works, I'm pretty sure we'll soon lose control (if
we haven't already; not all the developments are done in the open).
No. That door is wide open already — people can already do whatever
they like in _DSD properties. If they're going to screw up DT
properties, they'll screw up ACPI-only _DSD properties just the same.
You speak of maintaining "tight control of the _DSD properties that
are going to be used in ACPI tables"... but if you're going to
confiscate their crackpipe and stand over them while they work, you can
do that just as well whether they're using "PRP0001" or "FOO1234" as
their HID value.
In that sense, the HID is entirely orthogonal.
And think about it... we *really* don't want a given peripheral device
to have *different* bindings depending on whether it's discovered with
a specific ACPI HID, vs. when it's discovered via DT or the PRP0001
HID. That way lies complete insanity.
In some ways, your proposal would be actively *counterproductive*. You
say you want to train people *not* to keep patching the kernel. But
where they *could* have just used PRP0001 and used a pre-existing
kernel, you then tell them "oh, but now you need to patch the kernel
because we want you to make up a new HID and not be compatible with
what already exists."
If you go down this road, I predict we'll start seeing *separate*
drivers for identical components, because the bindings for DT vs. ACPI
properties are different. We really don't want that.
The pro ARM ACPI camp has been very vocal against DT in the server
space. I'd like to seem them as vocal about PRP0001, unless they find it
acceptable (and should apologise for bashing DT ;)).
Fundamentally, that DT vs. ACPI distinction has gone away with the
introduction of _DSD. We *need* the flexibility that we gain from being
able to provide device properties rather than inventing a new HID for
every permutation, and with that flexibility comes a certain amount of
responsibility to do things sensibly.
People have not always designed their bindings sensibly. But that isn't
going to be magically solved in the ACPI world, unless you *do*
actually stand over them with their crackpipe in your hand, as I joked
above. And eschewing PRP0001 really doesn't help you with that. It just
makes things harder.
--
dwmw2
From: Lorenzo Pieralisi <hidden> Date: 2015-09-24 14:01:57
On Thu, Sep 24, 2015 at 12:52:38PM +0100, David Woodhouse wrote:
[...]
And think about it... we *really* don't want a given peripheral device
to have *different* bindings depending on whether it's discovered with
a specific ACPI HID, vs. when it's discovered via DT or the PRP0001
HID. That way lies complete insanity.
There are "devices" that we do _not_ want to discover at all when
booting with ACPI (eg drivers/clk, drivers/regulator) because the way
they interact with ACPI standard power management AML methods may
definitely clash.
PRP0001 allows to pull everything in (depending on what the Coccinelle
script will look like, because AFAIK that's the only thing that stops
people from enabling eg drivers/clk drivers in ACPI systems with PRP0001,
namely the OF to FW node API conversion).
In some ways, your proposal would be actively *counterproductive*. You
say you want to train people *not* to keep patching the kernel. But
where they *could* have just used PRP0001 and used a pre-existing
kernel, you then tell them "oh, but now you need to patch the kernel
because we want you to make up a new HID and not be compatible with
what already exists."
We do not want people to reuse DT drivers that are handled in ACPI with
existing methods (again, power management is a prime example).
I do not think we want to enable DT PCI host controllers drivers either
since the ACPI model for PCI works differently already.
Should I mention CPUfreq and CPUidle DT drivers ?
The list of concerns goes on, it does not stop here.
If you go down this road, I predict we'll start seeing *separate*
drivers for identical components, because the bindings for DT vs. ACPI
properties are different. We really don't want that.
quoted
The pro ARM ACPI camp has been very vocal against DT in the server
space. I'd like to seem them as vocal about PRP0001, unless they find it
acceptable (and should apologise for bashing DT ;)).
Fundamentally, that DT vs. ACPI distinction has gone away with the
introduction of _DSD. We *need* the flexibility that we gain from being
able to provide device properties rather than inventing a new HID for
every permutation, and with that flexibility comes a certain amount of
responsibility to do things sensibly.
How do you enforce that apart from limiting the number of drivers you
convert to the FW node API ? What drivers are we going to convert and
on what basis ?
I think that's a debate worth having before making a tree-wide conversion
of OF API calls to the generic FW node API.
Thanks,
Lorenzo
On Thu, 2015-09-24 at 15:01 +0100, Lorenzo Pieralisi wrote:
On Thu, Sep 24, 2015 at 12:52:38PM +0100, David Woodhouse wrote:
[...]
quoted
And think about it... we *really* don't want a given peripheral device
to have *different* bindings depending on whether it's discovered with
a specific ACPI HID, vs. when it's discovered via DT or the PRP0001
HID. That way lies complete insanity.
There are "devices" that we do _not_ want to discover at all when
booting with ACPI (eg drivers/clk, drivers/regulator) because the way
they interact with ACPI standard power management AML methods may
definitely clash.
So it would be quite clearly a firmware bug to *expose* those things in
that way, when also exposing AML methods which make use of them.
But those really aren't what I'm looking at converting, anyway. The
target of the Coccinelle script would be leaf-node drivers, like the
one in this thread. Not subsystem code.
We have, for example, a generic method for 'get the GPIO' which works
equally well whether the actual information is given in the DT form, or
whether it's done through the ACPI GPIO abstraction as described in
Documentation/acpi/gpio-properties.txt.
We certainly shouldn't be looking at a naïve 1:1 literal
transliteration of properties. But on the other hand, we *do* want a
direct and automatic bijective mapping between the ACPI and DT
representations.
PRP0001 allows to pull everything in (depending on what the Coccinelle
script will look like, because AFAIK that's the only thing that stops
people from enabling eg drivers/clk drivers in ACPI systems with PRP0001,
namely the OF to FW node API conversion).
quoted
In some ways, your proposal would be actively *counterproductive*. You
say you want to train people *not* to keep patching the kernel. But
where they *could* have just used PRP0001 and used a pre-existing
kernel, you then tell them "oh, but now you need to patch the kernel
because we want you to make up a new HID and not be compatible with
what already exists."
We do not want people to reuse DT drivers that are handled in ACPI with
existing methods (again, power management is a prime example).
I do not think we want to enable DT PCI host controllers drivers either
since the ACPI model for PCI works differently already.
Should I mention CPUfreq and CPUidle DT drivers ?
No. Except to note that those are prime examples of the parts that
*don't* want to be converted.
quoted
Fundamentally, that DT vs. ACPI distinction has gone away with the
introduction of _DSD. We *need* the flexibility that we gain from being
able to provide device properties rather than inventing a new HID for
every permutation, and with that flexibility comes a certain amount of
responsibility to do things sensibly.
How do you enforce that apart from limiting the number of drivers you
convert to the FW node API ? What drivers are we going to convert and
on what basis ?
To start with, as I said, just leaf-node device drivers. None of the
things like the above, which you are quite right don't really want a
trivial conversion.
It's insane to talk of "enforcement", so let's not go there. Anyone
taking an existing driver and adding an ACPI HID to it would need to
start with *precisely* the same patches for those properties anyway.
Just as shown in the example in this thread.
I think that's a debate worth having before making a tree-wide conversion
of OF API calls to the generic FW node API.
Well, if that's true, that's a debate worth having before introducing
the generic FW node API. That having been kind of the *point* of the
generic FW node API — to give code which doesn't *need* to be
gratuitously firmware-specific, an API that works everywhere.
--
dwmw2
On Thu, Sep 24, 2015 at 12:52:38PM +0100, David Woodhouse wrote:
On Thu, 2015-09-24 at 11:31 +0100, Catalin Marinas wrote:
quoted
PRP0001 opens the door to any DT-like properties in ACPI and, knowing
how the ARM ecosystem works, I'm pretty sure we'll soon lose control (if
we haven't already; not all the developments are done in the open).
No. That door is wide open already — people can already do whatever
they like in _DSD properties. If they're going to screw up DT
properties, they'll screw up ACPI-only _DSD properties just the same.
_DSD isn't something I like either. But since we now allow it, we should
try to restrict its use to the bare minimum.
You speak of maintaining "tight control of the _DSD properties that
are going to be used in ACPI tables"... but if you're going to
confiscate their crackpipe and stand over them while they work, you can
do that just as well whether they're using "PRP0001" or "FOO1234" as
their HID value.
With "PRP0001", they can skip the _DSD properties review process (not
that they bother much currently) as long as the existing DT support
covers their needs. However, we don't want to emulate DT in ACPI but
first try the established ACPI methods and only use _DSD where these are
insufficient. Automatically converting existing drivers and encouraging
people to use "PRP0001" does not provide them with any incentive to try
harder and learn the "ACPI way".
In that sense, the HID is entirely orthogonal.
And think about it... we *really* don't want a given peripheral device
to have *different* bindings depending on whether it's discovered with
a specific ACPI HID, vs. when it's discovered via DT or the PRP0001
HID. That way lies complete insanity.
I'm fine with reusing the same property names between _DSD and DT but I
strongly consider that _DSD should only cover a _subset_ of the DT
bindings and they should be reviewed independently.
Take the smsc911x driver for example: the DT bindings for the ARM Juno
platform include things like "clocks", "vdd33a-supply". Such concepts
are not available in ACPI (but we've seen people trying to emulate
them), so rather than enabling such clocks in firmware, vendors will be
tempted to do things the DT way, possibly with "PRP0001" HIDs covering
the clock devices (though I think they currently require some kernel
changes).
In some ways, your proposal would be actively *counterproductive*. You
say you want to train people *not* to keep patching the kernel. But
where they *could* have just used PRP0001 and used a pre-existing
kernel, you then tell them "oh, but now you need to patch the kernel
because we want you to make up a new HID and not be compatible with
what already exists."
I gave an example above with the clocks but let's take a simpler,
device-specific property like "smsc,irq-active-high". Is this documented
anywhere apart from the kernel driver and the in-kernel DT smsc911x.txt?
No. Are other non-Linux OS vendors going to look in the kernel source
tree to implement support in their drivers? I doubt it and we could end
up with two different paths in firmware for handling the same device.
ACPI probably never was truly OS agnostic but with "PRP0001" we don't
even try.
The idea of registering a HID is to also have a process for getting
corresponding _DSD properties approved, published. Some of them like
"mac-address" would be more generic and not require further review but
we'll have more specific properties.
Patching a kernel driver to support a new HID is a (weak) method of
keeping track of which devices are used with ACPI. Given how quickly
these two patches were merged while still under discussion, I doubt that
arch maintainers would be in a position to review/reject drivers using
non-approved _DSD properties.
If you go down this road, I predict we'll start seeing *separate*
drivers for identical components, because the bindings for DT vs. ACPI
properties are different. We really don't want that.
What if Linux is not the first targeted OS? We either convince the
vendor to change the firmware or we'll have to support properties
invented for other operating system.
quoted
The pro ARM ACPI camp has been very vocal against DT in the server
space. I'd like to seem them as vocal about PRP0001, unless they find it
acceptable (and should apologise for bashing DT ;)).
Fundamentally, that DT vs. ACPI distinction has gone away with the
introduction of _DSD. We *need* the flexibility that we gain from
being able to provide device properties rather than inventing a new
HID for every permutation, and with that flexibility comes a certain
amount of responsibility to do things sensibly.
You may trust the x86 world to be responsible. I can't say the same
about the ARM SoC world ;).
While I understand the need for the flexibility of _DSD, I think the
main difference of opinion is that I do not consider ACPI+_DSD equal to
DT. It's fine to borrow some concepts from DT but I'm definitely against
emulating it entirely, which is pretty much what "PRP0001" is doing.
People have not always designed their bindings sensibly. But that
isn't going to be magically solved in the ACPI world, unless you *do*
actually stand over them with their crackpipe in your hand, as I joked
above. And eschewing PRP0001 really doesn't help you with that. It
just makes things harder.
I agree with you that _DSD already opened the door to even more "mess".
But, IMO, our approaches differ: you are trying to avoid such mess
affecting the kernel by hiding it behind "PRP0001"; what I'm trying is
to restrict (as much as I can) the mess developing in the early days of
ARM ACPI. I don't think either method will be fully successful, we may
just end up with a combination of the two.
Apart from a _DSD properties review process, what we need is (main) OS
vendors enforcing it, possibly with stricter ACPI test suite. Disabling
PRP0001 for ARM wouldn't solve the problem but at least it would make
people think about what _DSD properties they need registered for their
device (or I'm over optimistic and I should just stop caring ;)).
--
Catalin
On Thu, 2015-09-24 at 16:15 +0100, Catalin Marinas wrote:
With "PRP0001", they can skip the _DSD properties review process (not
that they bother much currently) as long as the existing DT support
covers their needs.
So no change there then. I take it the smsc911x bindings didn't go
through this mythical _DSD properties review process either, right?
However, we don't want to emulate DT in ACPI but > first try the
established ACPI methods and only use _DSD where these are
insufficient. Automatically converting existing drivers and encouraging
people to use "PRP0001" does not provide them with any incentive to try
harder and learn the "ACPI way".
But again, we're *not* looking at a simplistic transliteration of
properties. Where there *is* an "ACPI way", such as the GPIO example I
gave, that should absolutely be used. And there should be sufficiently
high-level access functions that it shouldn't *matter* to the driver
whether the information is coming from ACPI or DT.
quoted
In that sense, the HID is entirely orthogonal.
And think about it... we *really* don't want a given peripheral device
to have *different* bindings depending on whether it's discovered with
a specific ACPI HID, vs. when it's discovered via DT or the PRP0001
HID. That way lies complete insanity.
I'm fine with reusing the same property names between _DSD and DT but I
strongly consider that _DSD should only cover a _subset_ of the DT
bindings and they should be reviewed independently.
Take the smsc911x driver for example: the DT bindings for the ARM Juno
platform include things like "clocks", "vdd33a-supply". Such concepts
are not available in ACPI (but we've seen people trying to emulate
them), so rather than enabling such clocks in firmware, vendors will be
tempted to do things the DT way, possibly with "PRP0001" HIDs covering
the clock devices (though I think they currently require some kernel
changes).
Aren't those optional? If nothing is provided, they are basically a
no-op. AFAICT the existing patches for smsc911x don't touch that code
at all. So whatever motivation you imagine there is for ACPI firmware
vendors to "do things the DT way" is still there with the existing
patches.
Again, this is *orthogonal* to the question of whether the device is
matched by PRP0001 + its compatible string, vs. a newly-invented HID.
quoted
In some ways, your proposal would be actively *counterproductive*. You
say you want to train people *not* to keep patching the kernel. But
where they *could* have just used PRP0001 and used a pre-existing
kernel, you then tell them "oh, but now you need to patch the kernel
because we want you to make up a new HID and not be compatible with
what already exists."
I gave an example above with the clocks but let's take a simpler,
device-specific property like "smsc,irq-active-high". Is this documented
anywhere apart from the kernel driver and the in-kernel DT smsc911x.txt?
No. Are other non-Linux OS vendors going to look in the kernel source
tree to implement support in their drivers? I doubt it and we could end
up with two different paths in firmware for handling the same device.
ACPI probably never was truly OS agnostic but with "PRP0001" we don't
even try.
Where is the documentation for the INT_CFG_IRQ_POL_ bit in the INT_CFG
register, that it controls? The documentation on the bindings really
ought to live near that, in an ideal world.
But that's still a non-sequitur in the context of this particular
discussion. The patch that was posted *already* keeps that very same
(optional) smsc,irq-active-high property.
Again, it isn't relevant to the question of whether the driver is
matched via PRP0001 or a newly-allocated HID.
The driver, as the patch was posted, *does* have the same set of
properties with the same meanings — because anything else would be
insane.
The idea of registering a HID is to also have a process for getting
corresponding _DSD properties approved, published. Some of them like
"mac-address" would be more generic and not require further review but
we'll have more specific properties.
Patching a kernel driver to support a new HID is a (weak) method of
keeping track of which devices are used with ACPI. Given how quickly
these two patches were merged while still under discussion, I doubt that
arch maintainers would be in a position to review/reject drivers using
non-approved _DSD properties.
Again, nothing has changed there.
Apart from a _DSD properties review process, what we need is (main) OS
vendors enforcing it, possibly with stricter ACPI test suite. Disabling
PRP0001 for ARM wouldn't solve the problem but at least it would make
people think about what _DSD properties they need registered for their
device (or I'm over optimistic and I should just stop caring ;)).
I'm all for requiring pre-existing DT bindings to be "vetted" by the
nascent _DSD review process, before their use with PRP0001 can be
'blessed'.
But in a world where people *are* going to go off and do their own
insane thing, we really might as well let them use the *same* thing
that we already had — in as many cases as possible — rather than
actually *making* them come up with a new insane thing all of their
very own.
--
dwmw2
On Thu, Sep 24, 2015 at 07:10:38PM +0100, David Woodhouse wrote:
On Thu, 2015-09-24 at 16:15 +0100, Catalin Marinas wrote:
quoted
With "PRP0001", they can skip the _DSD properties review process (not
that they bother much currently) as long as the existing DT support
covers their needs.
So no change there then. I take it the smsc911x bindings didn't go
through this mythical _DSD properties review process either, right?
Right. And that's another reason I disagree with this patchset (I don't
always agree with everything that's coming out of ARM Ltd ;)).
Now, in Jeremy's defence, this _DSD process looks quite mythical indeed.
We had an ARM firmware ecosystem meeting earlier this year where we
agreed to set up such process (at least for the ARM world):
https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/CoreUpstreamNotes
I can see some form of a process here:
http://www.uefi.org/sites/default/files/resources/web-page-v2.pdf
But, at least to me, it's not clear how you submit properties to such
registry, what's the review process, who are the reviewers. I know there
was a mailing list set up but I don't see it listed above.
quoted
However, we don't want to emulate DT in ACPI but > first try the
established ACPI methods and only use _DSD where these are
insufficient. Automatically converting existing drivers and encouraging
people to use "PRP0001" does not provide them with any incentive to try
harder and learn the "ACPI way".
But again, we're *not* looking at a simplistic transliteration of
properties.
I know you and I don't look at it this way but, based on historical
evidence, I'm sure that some ARM vendors will try to do exactly this
(and be able to claim "ACPI support" when they were happily using DT).
quoted
quoted
In some ways, your proposal would be actively *counterproductive*. You
say you want to train people *not* to keep patching the kernel. But
where they *could* have just used PRP0001 and used a pre-existing
kernel, you then tell them "oh, but now you need to patch the kernel
because we want you to make up a new HID and not be compatible with
what already exists."
I gave an example above with the clocks but let's take a simpler,
device-specific property like "smsc,irq-active-high". Is this documented
anywhere apart from the kernel driver and the in-kernel DT smsc911x.txt?
No. Are other non-Linux OS vendors going to look in the kernel source
tree to implement support in their drivers? I doubt it and we could end
up with two different paths in firmware for handling the same device.
ACPI probably never was truly OS agnostic but with "PRP0001" we don't
even try.
Where is the documentation for the INT_CFG_IRQ_POL_ bit in the INT_CFG
register, that it controls? The documentation on the bindings really
ought to live near that, in an ideal world.
But that's still a non-sequitur in the context of this particular
discussion. The patch that was posted *already* keeps that very same
(optional) smsc,irq-active-high property.
Again, it isn't relevant to the question of whether the driver is
matched via PRP0001 or a newly-allocated HID.
The driver, as the patch was posted, *does* have the same set of
properties with the same meanings — because anything else would be
insane.
Having the same set of properties makes sense. But not documenting them
outside of Documentation/devicetree/bindings/ doesn't look right to me,
from an OS-agnostic ACPI perspective.
Let's take it the other way. A new driver is being submitted to Linux
(or another OS) with support for ACPI (only). The developers may find it
easier to use PRP0001 with their own _DSD invention. Do we ask the
developers to submit DT bindings even if they don't immediately target
DT (even harder if Linux is not the first target)? Or we skip this
properties review and documenting process altogether? At least with DT,
we usually see the bindings and kernel code together and have a chance
to NAK. For _DSD we need something outside the Linux kernel community.
But I agree with your statement above, it's not relevant whether a new
_HID is used or PRP0001 + "compatible" when we don't even control which
_DSD properties are added.
As I said previously, disabling PRP0001 on ARM is more of a weak method
of keeping track of which drivers are used with ACPI. My concern (and it
won't go away) is a thoughtless migration of existing DT drivers and ARM
SoCs to ACPI. PRP0001 makes it easier though it's _DSD actually
facilitating this.
quoted
Apart from a _DSD properties review process, what we need is (main) OS
vendors enforcing it, possibly with stricter ACPI test suite. Disabling
PRP0001 for ARM wouldn't solve the problem but at least it would make
people think about what _DSD properties they need registered for their
device (or I'm over optimistic and I should just stop caring ;)).
I'm all for requiring pre-existing DT bindings to be "vetted" by the
nascent _DSD review process, before their use with PRP0001 can be
'blessed'.
If we want something enforced here, we need a better methodology than
keeping track of which patches are submitted (and this would involve OS
vendors support since in many cases their kernels are seen as the
"canonical" implementation).
But in a world where people *are* going to go off and do their own
insane thing, we really might as well let them use the *same* thing
that we already had — in as many cases as possible — rather than
actually *making* them come up with a new insane thing all of their
very own.
This only works as long as they target an existing driver with prior DT
support (usually with reviewed bindings). If they have a new driver and
only ACPI in mind, I'm pretty sure we'll end up with new insane things.
That's why we need some form of _DSD properties review and "compatible"
is one such property.
--
Catalin
From: Rafael J. Wysocki <hidden> Date: 2015-09-26 02:16:31
On 9/25/2015 5:28 PM, Catalin Marinas wrote:
On Thu, Sep 24, 2015 at 07:10:38PM +0100, David Woodhouse wrote:
quoted
On Thu, 2015-09-24 at 16:15 +0100, Catalin Marinas wrote:
quoted
With "PRP0001", they can skip the _DSD properties review process (not
that they bother much currently) as long as the existing DT support
covers their needs.
So no change there then. I take it the smsc911x bindings didn't go
through this mythical _DSD properties review process either, right?
That was just an attempt and the final one will most likely be different.
But, at least to me, it's not clear how you submit properties to such
registry, what's the review process, who are the reviewers. I know there
was a mailing list set up but I don't see it listed above.
For the record, this is under discussion right now.
Also for the record, I'm not speaking for Intel. All of my comments
here are based on my personal opinions.
quoted
quoted
However, we don't want to emulate DT in ACPI but > first try the
established ACPI methods and only use _DSD where these are
insufficient. Automatically converting existing drivers and encouraging
people to use "PRP0001" does not provide them with any incentive to try
harder and learn the "ACPI way".
But again, we're *not* looking at a simplistic transliteration of
properties.
I know you and I don't look at it this way but, based on historical
evidence, I'm sure that some ARM vendors will try to do exactly this
(and be able to claim "ACPI support" when they were happily using DT).
quoted
quoted
quoted
In some ways, your proposal would be actively *counterproductive*. You
say you want to train people *not* to keep patching the kernel. But
where they *could* have just used PRP0001 and used a pre-existing
kernel, you then tell them "oh, but now you need to patch the kernel
because we want you to make up a new HID and not be compatible with
what already exists."
I gave an example above with the clocks but let's take a simpler,
device-specific property like "smsc,irq-active-high". Is this documented
anywhere apart from the kernel driver and the in-kernel DT smsc911x.txt?
No. Are other non-Linux OS vendors going to look in the kernel source
tree to implement support in their drivers? I doubt it and we could end
up with two different paths in firmware for handling the same device.
ACPI probably never was truly OS agnostic but with "PRP0001" we don't
even try.
Where is the documentation for the INT_CFG_IRQ_POL_ bit in the INT_CFG
register, that it controls? The documentation on the bindings really
ought to live near that, in an ideal world.
But that's still a non-sequitur in the context of this particular
discussion. The patch that was posted *already* keeps that very same
(optional) smsc,irq-active-high property.
Again, it isn't relevant to the question of whether the driver is
matched via PRP0001 or a newly-allocated HID.
The driver, as the patch was posted, *does* have the same set of
properties with the same meanings — because anything else would be
insane.
Having the same set of properties makes sense. But not documenting them
outside of Documentation/devicetree/bindings/ doesn't look right to me,
from an OS-agnostic ACPI perspective.
They are documented in the code too through the requirements that the
existing drivers have.
If I have a piece of hardware and a driver for it, it really shouldn't
matter whether I use a DT or ACPI in my platform. The driver should
just work in both cases. It doesn't have to work in exactly the same
way, but it should work. That's not the case today, sadly.
Let's take it the other way. A new driver is being submitted to Linux
(or another OS) with support for ACPI (only). The developers may find it
easier to use PRP0001 with their own _DSD invention. Do we ask the
developers to submit DT bindings even if they don't immediately target
DT (even harder if Linux is not the first target)? Or we skip this
properties review and documenting process altogether? At least with DT,
we usually see the bindings and kernel code together and have a chance
to NAK. For _DSD we need something outside the Linux kernel community.
But I agree with your statement above, it's not relevant whether a new
_HID is used or PRP0001 + "compatible" when we don't even control which
_DSD properties are added.
As I said previously, disabling PRP0001 on ARM is more of a weak method
of keeping track of which drivers are used with ACPI. My concern (and it
won't go away) is a thoughtless migration of existing DT drivers and ARM
SoCs to ACPI. PRP0001 makes it easier though it's _DSD actually
facilitating this.
Even if they tried to do that, infrastructure is missing for things like
regulators etc (because the frameworks there use of_ helpers directly
IIRC and I'm not aware of any plans to change that) and on the other
hand GPIO already depends on _CRS being used in accordance with ACPI
anyway (even if _DSD properties are used), so I'd say this would be
practically difficult rather.
IOW, using _DSD properties for individual devices/drivers is easy. Doing
the same thing for the whole system, not so much.
quoted
quoted
Apart from a _DSD properties review process, what we need is (main) OS
vendors enforcing it, possibly with stricter ACPI test suite. Disabling
PRP0001 for ARM wouldn't solve the problem but at least it would make
people think about what _DSD properties they need registered for their
device (or I'm over optimistic and I should just stop caring ;)).
I'm all for requiring pre-existing DT bindings to be "vetted" by the
nascent _DSD review process, before their use with PRP0001 can be
'blessed'.
If we want something enforced here, we need a better methodology than
keeping track of which patches are submitted (and this would involve OS
vendors support since in many cases their kernels are seen as the
"canonical" implementation).
quoted
But in a world where people *are* going to go off and do their own
insane thing, we really might as well let them use the *same* thing
that we already had — in as many cases as possible — rather than
actually *making* them come up with a new insane thing all of their
very own.
This only works as long as they target an existing driver with prior DT
support (usually with reviewed bindings).
Exactly.
And this is what we (David and me at least) want to be possible. It
would be a failure if people had to write separate drivers for
ACPI-based and DT-based platforms to handle the same hardware, at least
at the leaf driver level.
If they have a new driver and
only ACPI in mind, I'm pretty sure we'll end up with new insane things.
We will. With or without _DSD and all. That had happened way before
_DSD was introduced.
And by the way, demonizing _DSD doesn't help you at all, because there
was the *much* *worse* thing called _DSM way before and it allowed of
the same things as _DSD does and more. And it still is there and it
very well may be used to feed an entire DT in binary format to the
kernel when no one is looking.
So this is all about tradeoffs. If you don't give people enough rope to
do something, they'll find another way and you may hate that one, but
then it'll be too late.
That's why we need some form of _DSD properties review and "compatible"
is one such property.
Yes, we need and want to set up a process for registering _DSD
properties. That turns out a bit more complicated than we thought it
would be, though.
No, that won't prevent people from doing insane things with properties
(or similar) they never register and they support in their
out-of-the-tree drivers they never attempt to mainline. But let's be
honest that this can happen with DT just as well.
And it seems to me that we're spending more time on doing politics than
on solving technical problems which should really be our focus IMO.
And the underlying technical problem to me is what I said before: Device
drivers should work regardless of what way the configuration information
is provided to the kernel. Indeed, they shouldn't even have to care
about that.
Do you agree with that?
Thanks,
Rafael
On Fri, 2015-09-25 at 16:28 +0100, Catalin Marinas wrote:
This only works as long as they target an existing driver with prior DT
support (usually with reviewed bindings). If they have a new driver and
only ACPI in mind, I'm pretty sure we'll end up with new insane things.
That's why we need some form of _DSD properties review and "compatible"
is one such property.
Sure, that makes a lot of sense.
My main concern is that we don't end up with gratuitously *different*
property sets for DT vs. ACPI. That way we end up with either two
separate drivers, or abstracting the core of the driver out and having
two bindings for it (much as we do for PCI vs. platform/etc for some
devices already). We don't want that pain where we can avoid it. And we
don't want people to *have* to hack the kernel driver to migrate to
ACPI, if we can avoid it.
Sometimes it might be worth being different — if the DT binding is
utterly crap, and deserves to be thrown away. In that case, by all
means invent a new binding. But if we're going to accept the pain of
having multiple bindings, why not make the *new* one work via DT too
anyway.
I'd be happy to see the existing DT bindings put through the nascent
_DSD review process — such as it is — and into the database. One at a
time on a case-by-case basis as they get used, perhaps.
--
dwmw2
Adding Charles Garcia-Tobin from ARM....
On 09/25/2015 09:28 AM, Catalin Marinas wrote:
On Thu, Sep 24, 2015 at 07:10:38PM +0100, David Woodhouse wrote:
quoted
On Thu, 2015-09-24 at 16:15 +0100, Catalin Marinas wrote:
quoted
With "PRP0001", they can skip the _DSD properties review process (not
that they bother much currently) as long as the existing DT support
covers their needs.
So no change there then. I take it the smsc911x bindings didn't go
through this mythical _DSD properties review process either, right?
Right. And that's another reason I disagree with this patchset (I don't
always agree with everything that's coming out of ARM Ltd ;)).
Now, in Jeremy's defence, this _DSD process looks quite mythical indeed.
We had an ARM firmware ecosystem meeting earlier this year where we
agreed to set up such process (at least for the ARM world):
https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/CoreUpstreamNotes
I can see some form of a process here:
http://www.uefi.org/sites/default/files/resources/web-page-v2.pdf
But, at least to me, it's not clear how you submit properties to such
registry, what's the review process, who are the reviewers. I know there
was a mailing list set up but I don't see it listed above.
To repeat Rafael a bit, getting the process set up -- moving it from the
mythical to the practical -- is taking longer than expected. Some folks
that had agreed to do so made no progress at all for many months, but I
was way too polite and waited for something to happen.
So, it turns out I'm impatient at times. This _DSD stuff is bugging me and
the itch needs scratching. And, it absolutely affects ACPI on ARM servers
so it has my interest. I have thrown out some basic proposals, and in
conjunction with the ASWG (ACPI Spec Working Group of UEFI), Rafael, and
Charles, we're trying to get some things decided and moving.
Here is where I think we are:
-- We have defined a very clear boundary between ASWG and the definition
of _DSD device properties. The bottom line is that all device property
definition is outside the scope of the ASWG; the ASWG has defined the
structure they must take in ASL, but the actual content of any _DSD
we can discuss and define in public forums without having to concern
ourselves about the spec in any way. This was an essential step in
order to make IP rules clear.
-- There is a mailing list set up (dsd@acpica.org), originally set up as
the starting point for a review process. I have recently started up
some discussion there on what needs to be submitted. I will soon be
posting updates to the discussion to lay out proposed review processes
and the proposed formalisms to be used in describing device properties
for review. Please subscribe and read the archives (not very large)
that can be found here: https://lists.acpica.org/mailman/listinfo/dsd
In the meantime, device property definition review can start occurring
on the dsd@acpica.org list with the caveat that process is still being
defined. At least if things are put in the email archive, we've got a
starting point.
-- Unfortunately, the material currently on the UEFI site about _DSD (other
than its definition) should be ignored -- it reflects much earlier views
and was incomplete to begin with. I wrote what's there and have no qualms
in saying it is now inadequate. This is being corrected but it needs to
go through ASWG to be fixed.
-- An initial draft of a proposal for a metalanguage for describing and
storing device properties has been under review. I'm working out v2 of
this formalism. This will allow us to document the device properties in
a reliable way, and build a repository of the definitions that is easily
shared.
-- What documentation/proposals we do have I have stashed in github. Please
see https://github.com/ahs3/dsd/tree/v-next/ for the work in progress.
What needs to happen next?
-- First, finalize the review process (discussion on dsd@apcica.org).
-- Second, finalize the reviewers (discussion on dsd@).
-- Third, but in parallel, finalize formalisms for describing device
properties and build the tools needs to validate them, store them,
maintain them, and store them for public consumption (discussion
also on dsd@).
-- Make a couple of decisions (see below...)
quoted
quoted
However, we don't want to emulate DT in ACPI but > first try the
established ACPI methods and only use _DSD where these are
insufficient. Automatically converting existing drivers and encouraging
people to use "PRP0001" does not provide them with any incentive to try
harder and learn the "ACPI way".
But again, we're *not* looking at a simplistic transliteration of
properties.
I know you and I don't look at it this way but, based on historical
evidence, I'm sure that some ARM vendors will try to do exactly this
(and be able to claim "ACPI support" when they were happily using DT).
Based on the discussion so far, I think everyone is agreeing that we
do NOT want to just blindly reproduce DT using _DSD properties. Is that
a fair statement?
quoted
quoted
quoted
In some ways, your proposal would be actively *counterproductive*. You
say you want to train people *not* to keep patching the kernel. But
where they *could* have just used PRP0001 and used a pre-existing
kernel, you then tell them "oh, but now you need to patch the kernel
because we want you to make up a new HID and not be compatible with
what already exists."
I gave an example above with the clocks but let's take a simpler,
device-specific property like "smsc,irq-active-high". Is this documented
anywhere apart from the kernel driver and the in-kernel DT smsc911x.txt?
No. Are other non-Linux OS vendors going to look in the kernel source
tree to implement support in their drivers? I doubt it and we could end
up with two different paths in firmware for handling the same device.
ACPI probably never was truly OS agnostic but with "PRP0001" we don't
even try.
Where is the documentation for the INT_CFG_IRQ_POL_ bit in the INT_CFG
register, that it controls? The documentation on the bindings really
ought to live near that, in an ideal world.
A common location has some other rules, too: (1) people from Microsoft
or other OS vendors (e.g., BSDs) need to be able to easily access it,
and (2) firmware developers needs to be able to easily find it and use
it. My thinking today is that I can (and will) set up a repository in
the github I'm using that will contain a human readable formal description
of all the approved device properties. Further (thanks to Mark Brown for
the idea), there needs to be a second repository co-located with the first
that contains device properties discovered in the wild but never "approved."
I'll even offer to maintain these repositories for the time being, until
we as a community decide on something better.
quoted
But that's still a non-sequitur in the context of this particular
discussion. The patch that was posted *already* keeps that very same
(optional) smsc,irq-active-high property.
Again, it isn't relevant to the question of whether the driver is
matched via PRP0001 or a newly-allocated HID.
The driver, as the patch was posted, *does* have the same set of
properties with the same meanings — because anything else would be
insane.
Having the same set of properties makes sense. But not documenting them
outside of Documentation/devicetree/bindings/ doesn't look right to me,
from an OS-agnostic ACPI perspective.
Let's take it the other way. A new driver is being submitted to Linux
(or another OS) with support for ACPI (only). The developers may find it
easier to use PRP0001 with their own _DSD invention. Do we ask the
developers to submit DT bindings even if they don't immediately target
DT (even harder if Linux is not the first target)? Or we skip this
properties review and documenting process altogether? At least with DT,
we usually see the bindings and kernel code together and have a chance
to NAK. For _DSD we need something outside the Linux kernel community.
But I agree with your statement above, it's not relevant whether a new
_HID is used or PRP0001 + "compatible" when we don't even control which
_DSD properties are added.
As I said previously, disabling PRP0001 on ARM is more of a weak method
of keeping track of which drivers are used with ACPI. My concern (and it
won't go away) is a thoughtless migration of existing DT drivers and ARM
SoCs to ACPI. PRP0001 makes it easier though it's _DSD actually
facilitating this.
I have to agree with Catalin here. I was on the ASWG when _DSD was proposed,
and when PRP0001 was proposed and they seemed reasonable at the time. However,
the more I think about PRP0001, the more I think I dislike it. It allows for
short-circuiting any sort of review about device properties in the drivers,
regardless of whether they support DT or ACPI or both -- and people will choose
the path of least resistance given any choice at all.
quoted
quoted
Apart from a _DSD properties review process, what we need is (main) OS
vendors enforcing it, possibly with stricter ACPI test suite. Disabling
PRP0001 for ARM wouldn't solve the problem but at least it would make
people think about what _DSD properties they need registered for their
device (or I'm over optimistic and I should just stop caring ;)).
It's not foolproof, but if we disallow PRP0001 we might be able to encourage
some sort of review to occur before code gets dropped into the kernel where
it will need to be maintained forever.
So, here's another decision point: do we disallow the use of PRP0001 for device
properties on ARMv8 servers using ACPI? And should other architectures be able
to do as they wish with PRP0001? The previous discussion seems to say "yes" to
both questions, correct?
quoted
I'm all for requiring pre-existing DT bindings to be "vetted" by the
nascent _DSD review process, before their use with PRP0001 can be
'blessed'.
If we want something enforced here, we need a better methodology than
keeping track of which patches are submitted (and this would involve OS
vendors support since in many cases their kernels are seen as the
"canonical" implementation).
Agreed. This is what Charles, Rafael and I have started discussing on the
dsd@acpica.org mailing list. I have proposed a formalism for describing
properties (actively being revised) and we have some basic process proposals
being discussed. More voices are needed; we're just starting to get things
moving after they've been in stasis so long.
While I cannot speak for Red Hat as a whole, for myself I really want for there
to be a single place I can go to where I can find out which device properties
are actually defined, which are not, and to be able to point to when some random
bit of hardware or firmware decides to do something completely different and
breaks my OS. Help Charles, Rafael and I define the vetting process and the
things to be vetted and I think we can have a decent methodology in place.
quoted
But in a world where people *are* going to go off and do their own
insane thing, we really might as well let them use the *same* thing
that we already had — in as many cases as possible — rather than
actually *making* them come up with a new insane thing all of their
very own.
This only works as long as they target an existing driver with prior DT
support (usually with reviewed bindings). If they have a new driver and
only ACPI in mind, I'm pretty sure we'll end up with new insane things.
That's why we need some form of _DSD properties review and "compatible"
is one such property.
Again, based on the emails so far, it seems everyone is agreeing that we
need a review process, and that we need to make sure we don't accept anything
into the kernel that has not been reviewed; it won't fix everything, but it
at least gives us a fighting chance. Any disagreement with that?
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
From: Charles Garcia-Tobin <hidden> Date: 2015-10-06 00:20:49
On 01/10/2015 03:23, "ahs3@redhat.com" [off-list ref] wrote:
Adding Charles Garcia-Tobin from ARM....
Thanks and sorry for chiming in late. You¹ve pretty much said it all, but
I had some comments. First one, is that if folk want to see what¹s
happening with the DSD process, then please join the dsd mailing list and
read this posting :
[DSD] DSD v2
<https://lists.acpica.org/pipermail/dsd/2015-September/000026.html>
To me this thread reads like there is a lot of violent agreement on what
_DSD should not be used for, and on not breaking existing ACPI<->kernel
interfaces. This is good, and essentially what we tried to capture in the
_DSD review process. However, a lot of concern does remain on PRP0001. I
share the worry that PRP00001 facilitates ³thoughtless migration² of
existing DT to _DSD. I understand the need for porting, but above all ACPI
is an OS agnostic standard, and whilst PRP0001 does provide porting
convenience, I don¹t think we should be looking at it in ACPI circles
unless we had wider agreement among OSs to use it. AFAIK PRP00001 has not
actually been approved yet in the specification forum, and that it in
itself is more of a concern for me, as the code has been pushed upstream.
I guess it¹s up to Catalin, but disabling for ARM seems like a good idea
right now, another option is to add tests to FWTS.
Cheers
Charles
On 09/25/2015 09:28 AM, Catalin Marinas wrote:
quoted
On Thu, Sep 24, 2015 at 07:10:38PM +0100, David Woodhouse wrote:
quoted
On Thu, 2015-09-24 at 16:15 +0100, Catalin Marinas wrote:
quoted
With "PRP0001", they can skip the _DSD properties review process (not
that they bother much currently) as long as the existing DT support
covers their needs.
So no change there then. I take it the smsc911x bindings didn't go
through this mythical _DSD properties review process either, right?
Right. And that's another reason I disagree with this patchset (I don't
always agree with everything that's coming out of ARM Ltd ;)).
Now, in Jeremy's defence, this _DSD process looks quite mythical indeed.
We had an ARM firmware ecosystem meeting earlier this year where we
agreed to set up such process (at least for the ARM world):
https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/CoreUpstreamNotes
I can see some form of a process here:
http://www.uefi.org/sites/default/files/resources/web-page-v2.pdf
But, at least to me, it's not clear how you submit properties to such
registry, what's the review process, who are the reviewers. I know there
was a mailing list set up but I don't see it listed above.
To repeat Rafael a bit, getting the process set up -- moving it from the
mythical to the practical -- is taking longer than expected. Some folks
that had agreed to do so made no progress at all for many months, but I
was way too polite and waited for something to happen.
So, it turns out I'm impatient at times. This _DSD stuff is bugging me
and
the itch needs scratching. And, it absolutely affects ACPI on ARM servers
so it has my interest. I have thrown out some basic proposals, and in
conjunction with the ASWG (ACPI Spec Working Group of UEFI), Rafael, and
Charles, we're trying to get some things decided and moving.
Here is where I think we are:
-- We have defined a very clear boundary between ASWG and the
definition
of _DSD device properties. The bottom line is that all device
property
definition is outside the scope of the ASWG; the ASWG has defined
the
structure they must take in ASL, but the actual content of any _DSD
we can discuss and define in public forums without having to concern
ourselves about the spec in any way. This was an essential step in
order to make IP rules clear.
-- There is a mailing list set up (dsd@acpica.org), originally set up
as
the starting point for a review process. I have recently started up
some discussion there on what needs to be submitted. I will soon be
posting updates to the discussion to lay out proposed review
processes
and the proposed formalisms to be used in describing device
properties
for review. Please subscribe and read the archives (not very large)
that can be found here:
https://lists.acpica.org/mailman/listinfo/dsd
In the meantime, device property definition review can start
occurring
on the dsd@acpica.org list with the caveat that process is still
being
defined. At least if things are put in the email archive, we've
got a
starting point.
-- Unfortunately, the material currently on the UEFI site about _DSD
(other
than its definition) should be ignored -- it reflects much earlier
views
and was incomplete to begin with. I wrote what's there and have no
qualms
in saying it is now inadequate. This is being corrected but it
needs to
go through ASWG to be fixed.
-- An initial draft of a proposal for a metalanguage for describing and
storing device properties has been under review. I'm working out
v2 of
this formalism. This will allow us to document the device
properties in
a reliable way, and build a repository of the definitions that is
easily
shared.
-- What documentation/proposals we do have I have stashed in github.
Please
see https://github.com/ahs3/dsd/tree/v-next/ for the work in
progress.
What needs to happen next?
-- First, finalize the review process (discussion on dsd@apcica.org).
-- Second, finalize the reviewers (discussion on dsd@).
-- Third, but in parallel, finalize formalisms for describing device
properties and build the tools needs to validate them, store them,
maintain them, and store them for public consumption (discussion
also on dsd@).
-- Make a couple of decisions (see below...)
quoted
quoted
quoted
However, we don't want to emulate DT in ACPI but > first try the
established ACPI methods and only use _DSD where these are
insufficient. Automatically converting existing drivers and
encouraging
people to use "PRP0001" does not provide them with any incentive to
try
harder and learn the "ACPI way".
But again, we're *not* looking at a simplistic transliteration of
properties.
I know you and I don't look at it this way but, based on historical
evidence, I'm sure that some ARM vendors will try to do exactly this
(and be able to claim "ACPI support" when they were happily using DT).
Based on the discussion so far, I think everyone is agreeing that we
do NOT want to just blindly reproduce DT using _DSD properties. Is that
a fair statement?
quoted
quoted
quoted
quoted
In some ways, your proposal would be actively *counterproductive*.
You
say you want to train people *not* to keep patching the kernel. But
where they *could* have just used PRP0001 and used a pre-existing
kernel, you then tell them "oh, but now you need to patch the kernel
because we want you to make up a new HID and not be compatible with
what already exists."
I gave an example above with the clocks but let's take a simpler,
device-specific property like "smsc,irq-active-high". Is this
documented
anywhere apart from the kernel driver and the in-kernel DT
smsc911x.txt?
No. Are other non-Linux OS vendors going to look in the kernel source
tree to implement support in their drivers? I doubt it and we could
end
up with two different paths in firmware for handling the same device.
ACPI probably never was truly OS agnostic but with "PRP0001" we don't
even try.
Where is the documentation for the INT_CFG_IRQ_POL_ bit in the INT_CFG
register, that it controls? The documentation on the bindings really
ought to live near that, in an ideal world.
A common location has some other rules, too: (1) people from Microsoft
or other OS vendors (e.g., BSDs) need to be able to easily access it,
and (2) firmware developers needs to be able to easily find it and use
it. My thinking today is that I can (and will) set up a repository in
the github I'm using that will contain a human readable formal description
of all the approved device properties. Further (thanks to Mark Brown for
the idea), there needs to be a second repository co-located with the first
that contains device properties discovered in the wild but never
"approved."
I'll even offer to maintain these repositories for the time being, until
we as a community decide on something better.
quoted
quoted
But that's still a non-sequitur in the context of this particular
discussion. The patch that was posted *already* keeps that very same
(optional) smsc,irq-active-high property.
Again, it isn't relevant to the question of whether the driver is
matched via PRP0001 or a newly-allocated HID.
The driver, as the patch was posted, *does* have the same set of
properties with the same meanings ‹ because anything else would be
insane.
Having the same set of properties makes sense. But not documenting them
outside of Documentation/devicetree/bindings/ doesn't look right to me,
from an OS-agnostic ACPI perspective.
Let's take it the other way. A new driver is being submitted to Linux
(or another OS) with support for ACPI (only). The developers may find it
easier to use PRP0001 with their own _DSD invention. Do we ask the
developers to submit DT bindings even if they don't immediately target
DT (even harder if Linux is not the first target)? Or we skip this
properties review and documenting process altogether? At least with DT,
we usually see the bindings and kernel code together and have a chance
to NAK. For _DSD we need something outside the Linux kernel community.
But I agree with your statement above, it's not relevant whether a new
_HID is used or PRP0001 + "compatible" when we don't even control which
_DSD properties are added.
As I said previously, disabling PRP0001 on ARM is more of a weak method
of keeping track of which drivers are used with ACPI. My concern (and it
won't go away) is a thoughtless migration of existing DT drivers and ARM
SoCs to ACPI. PRP0001 makes it easier though it's _DSD actually
facilitating this.
I have to agree with Catalin here. I was on the ASWG when _DSD was
proposed,
and when PRP0001 was proposed and they seemed reasonable at the time.
However,
the more I think about PRP0001, the more I think I dislike it. It allows
for
short-circuiting any sort of review about device properties in the
drivers,
regardless of whether they support DT or ACPI or both -- and people will
choose
the path of least resistance given any choice at all.
quoted
quoted
quoted
Apart from a _DSD properties review process, what we need is (main) OS
vendors enforcing it, possibly with stricter ACPI test suite.
Disabling
PRP0001 for ARM wouldn't solve the problem but at least it would make
people think about what _DSD properties they need registered for their
device (or I'm over optimistic and I should just stop caring ;)).
It's not foolproof, but if we disallow PRP0001 we might be able to
encourage
some sort of review to occur before code gets dropped into the kernel
where
it will need to be maintained forever.
So, here's another decision point: do we disallow the use of PRP0001 for
device
properties on ARMv8 servers using ACPI? And should other architectures
be able
to do as they wish with PRP0001? The previous discussion seems to say
"yes" to
both questions, correct?
quoted
quoted
I'm all for requiring pre-existing DT bindings to be "vetted" by the
nascent _DSD review process, before their use with PRP0001 can be
'blessed'.
If we want something enforced here, we need a better methodology than
keeping track of which patches are submitted (and this would involve OS
vendors support since in many cases their kernels are seen as the
"canonical" implementation).
Agreed. This is what Charles, Rafael and I have started discussing on the
dsd@acpica.org mailing list. I have proposed a formalism for describing
properties (actively being revised) and we have some basic process
proposals
being discussed. More voices are needed; we're just starting to get
things
moving after they've been in stasis so long.
While I cannot speak for Red Hat as a whole, for myself I really want for
there
to be a single place I can go to where I can find out which device
properties
are actually defined, which are not, and to be able to point to when some
random
bit of hardware or firmware decides to do something completely different
and
breaks my OS. Help Charles, Rafael and I define the vetting process and
the
things to be vetted and I think we can have a decent methodology in place.
quoted
quoted
But in a world where people *are* going to go off and do their own
insane thing, we really might as well let them use the *same* thing
that we already had ‹ in as many cases as possible ‹ rather than
actually *making* them come up with a new insane thing all of their
very own.
This only works as long as they target an existing driver with prior DT
support (usually with reviewed bindings). If they have a new driver and
only ACPI in mind, I'm pretty sure we'll end up with new insane things.
That's why we need some form of _DSD properties review and "compatible"
is one such property.
Again, based on the emails so far, it seems everyone is agreeing that we
need a review process, and that we need to make sure we don't accept
anything
into the kernel that has not been reviewed; it won't fix everything, but
it
at least gives us a fighting chance. Any disagreement with that?
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
On Mon, 2015-10-05 at 17:20 -0700, Charles Garcia-Tobin wrote:
it in ACPI circles
unless we had wider agreement among OSs to use it. AFAIK PRP00001 has not
actually been approved yet in the specification forum, and that it in
itself is more of a concern for me,as the code has been pushed upstream.
Why would that be a concern? In that context it's just one device ID.
Individual devices don't *need* to be approved. OK, the 'PRP' vendor
prefix is not officially assigned but that's really a trivial piece of
bureaucracy.
I guess it¹s up to Catalin, but disabling for ARM seems like a good idea
right now, another option is to add tests to FWTS.
I understand the motivation to avoid embracing a whole bunch of crappy
bindings. But I think that eschewing PRP0001 is the wrong technical
approach to achieving that.
It has false negatives — as soon as you have a *single* existing DT
binding, perhaps something as simple as the serial port bindings from
the CHRP days, you'll be in a situation where you can't use that.
I've *got* hardware where I need to advertise a serial port with a
clock-frequency property because it *isn't* compatible with PNP0501.
And it has false positives — there's nothing to prevent people from
doing ACPI-style bindings with crappy device bindings which also aren't
approved.
I think it's utterly naïve to believe that simply avoiding the use of
PRP0001 + compatible for matching is going to have *any* significant
beneficial effect whatsoever. It only makes life harder for all
concerned.
Perhaps a better approach would be to introduce something like
CONFIG_UNAPPROVED_BINDINGS (which can't be set on ARM64), and those
drivers which use bindings that *aren't* approved by Catalin's crack
team of reviewers need to depend on !UNAPPROVED_BINDINGS. To be honest,
I still think even *that* is somewhat naïve, but it's still a better
way of implementing what you're actually trying to achieve, however
optimistic you have to be to think it'll ever work in practice.
--
dwmw2
From: Rafael J. Wysocki <hidden> Date: 2015-10-08 00:11:38
On 10/6/2015 1:08 PM, David Woodhouse wrote:
On Mon, 2015-10-05 at 17:20 -0700, Charles Garcia-Tobin wrote:
quoted
it in ACPI circles
unless we had wider agreement among OSs to use it. AFAIK PRP00001 has not
actually been approved yet in the specification forum, and that it in
itself is more of a concern for me,as the code has been pushed upstream.
Why would that be a concern? In that context it's just one device ID.
Individual devices don't *need* to be approved. OK, the 'PRP' vendor
prefix is not officially assigned but that's really a trivial piece of
bureaucracy.
quoted
I guess it¹s up to Catalin, but disabling for ARM seems like a good idea
right now, another option is to add tests to FWTS.
I understand the motivation to avoid embracing a whole bunch of crappy
bindings. But I think that eschewing PRP0001 is the wrong technical
approach to achieving that.
It has false negatives — as soon as you have a *single* existing DT
binding, perhaps something as simple as the serial port bindings from
the CHRP days, you'll be in a situation where you can't use that.
I've *got* hardware where I need to advertise a serial port with a
clock-frequency property because it *isn't* compatible with PNP0501.
And it has false positives — there's nothing to prevent people from
doing ACPI-style bindings with crappy device bindings which also aren't
approved.
I think it's utterly naïve to believe that simply avoiding the use of
PRP0001 + compatible for matching is going to have *any* significant
beneficial effect whatsoever. It only makes life harder for all
concerned.
Perhaps a better approach would be to introduce something like
CONFIG_UNAPPROVED_BINDINGS (which can't be set on ARM64), and those
drivers which use bindings that *aren't* approved by Catalin's crack
team of reviewers need to depend on !UNAPPROVED_BINDINGS. To be honest,
I still think even *that* is somewhat naïve, but it's still a better
way of implementing what you're actually trying to achieve, however
optimistic you have to be to think it'll ever work in practice.
Also, let me mention one case PRP0001 is intended for that seems to be real.
Say there is a piece of hardware that becomes so popular that the
majority of vendors include it in their SoCs. Now all of them have to
allocate ACPI/PNP device IDs for that and (without PRP0001) all of those
device IDs need to go into the driver for that thing to make it work on
all of the SoCs in question. As a result, the mainline kernel doesn't
work on new SoCs with ACPI without modifications, although it works on
all of them with DT in principle. Moreover, unmodified binary distro
kernels don't work on new SoCs with ACPI, although they may work just
fine on them with DT (that sort of seems to be a case that Red Hat may
be interested in for one example, but I may be wrong here).
In theory, that may be addressed by allocating a "master" ACPI/PNP
device ID for that device and listing it in the device's _CID on all of
the SoCs in question, but then it isn't particularly clear who's going
to be responsible for allocating that device ID and "propagating" the
knowledge of it to everybody interested to make things consistent.
[Side note: Using a different vendor's device ID in a _CID is quite
questionable, so the "master" one would need to be something "vendor
agnostic" if that makes sense at all ...]
Let alone the fact that PRP0001 is actually quite useful at the
prototyping stage when it is premature to allocate a new device ID just
yet. Taking that to the extreme, if someone whittles a board in his or
her garage and wants to use it to drive their homemade grass watering
system, having to invent a new device ID and put it into the existing
driver that otherwise doesn't require any modifications is ... you know
what I mean.
All in all, I'd recommend some caution to ensure that the baby is not
going away along with the bath water here.
Thanks,
Rafael
From: Jeremy Linton <hidden> Date: 2015-11-02 15:48:06
On 09/09/2015 11:10 AM, Marc Zyngier wrote:
Jeremy,
I can see two issues here: we have a screaming interrupt, and
we seem to corrupt some workqueue.
How did you get this to work? Firmware release?
Marc,
I'm responding because its been a month or so since my last response,
and I haven't forgotten about this issue.
First, any custom tianocore build (*) should work. The required changes
have been in the last few linaro snapshots as well
(http://snapshots.linaro.org/components/kernel/linaro-edk2/, currently
at 40) but I personally haven't had a lot of luck with the prebuilt
images due to problems unrelated to this change. Others may have more luck.
* For those that don't know, tianno core is at:
https://github.com/tianocore/edk2.git
Use the master branch
After setting the environment variables/dependencies appropriately:
make -f ArmPlatformPkg/ArmJunoPkg/Makefile all
Will create a functional ACPI firmware image for all recent kernels,
including ACPI/PCIe ones.
Thanks for everyone's patience on this,