On Thu, 2016-09-22 at 12:39 -0400, Abdulhamid, Harb wrote:
On 9/20/2016 1:51 PM, Mark Rutland wrote:
>
> On Tue, Sep 20, 2016 at 12:42:08PM -0500, Timur Tabi wrote:
> >
> > Mark Rutland wrote:
> > >
> > > I think that this hints at a larger problem.
> > >
> > > If we're simply going to copy each DT bindings into DSD, then
> > > there
> > > isn't much point in reviewing DSD bindings in this manner.
> > > Similarly for
> > > devicetree review if DSD bindings are expected to also be taken
> > > as DT
> > > bindings.
> > >
> > > If DSD and DT bindings are going to have separate reviewers,
> > > and/or need
> > > to be idiomatically different, then the current approach taken
> > > in Linux
> > > of trying to hide the distinction behind unified property
> > > accessors is a
> > > false abstraction.
> > I was under the impression that the only reason the
> > device_property_read_xxx() functions exist is so that we can use
> > the
> > same property names for DT and ACPI.
> ... which is why I implied above (as I have stated previously) that
> this
> is a false abstraction.
I think this might be going on a bit of a tangent from the current
DSD
review, and turning into a broader philosophical discussion on why
DSD
and this database exists... which I guess is expected with the first
DSD
submission. :)
Yes. And Yes. We need to have the discussion. Appreciate everyone's
input.
I'm not sure what you mean by false abstraction. DSD properties must
be
supported by multiple operating systems, and is intended be OS
agnostic
(e.g. the exact same ACPI firmware is used both by all supported
operating systems). That is not currently the case for DT, hence the
reason why we need a separate review process. What specifically are
you
referring to as a "false abstraction".
In this particular case, we are extending an existing driver that
originally only supported DT to also support ACPI. But that will not
always be the case, and we should look at things on a case-by-case
basis.
In this case, if there is a property that can't translate between DT
and DSD, we need to create abstractions. We did this with GPIO, for
example.
To break down the logic in why we chose to use the existing property
name . In developing QUP I2C drivers for other operating systems, we
had decided to make the DSD properties match the DT property because:
1) This DT property for upstream Linux driver already existed, has
already been reviewed, and made sense to reuse the existing property
name.
2) It is possible to change the name for DSD without changing the
name
of the DT binding (even though they are used for the exact same
thing),
but in my opinion that is a pretty ugly solution, and I foresee
getting
push back.
3) Modifying the QUP I2C driver for other operating systems is far
less
painful. For better or worse, "upstreaming" changes other OS drivers
does not have to go through the same level scrutiny, nor is there
issues
with DT vs. ACPI biases.
>
>
> >
> > If that's true, then I think DSDs need to conform to DT
> > guidelines.
> > We should just officially claim that DSD is the method for
> > getting
> > DT properties into ACPI so that the device_property_read_xxx()
> > API
> > can be used. That makes things easier for everyone.
> I don't think that matters are quite that simple, and I disagree in
> general with the approach.
Agree, Timur's above statement is an oversimplification. While
device_property_read_xxx() is intended for unified abstraction for
acquiring device properties for device drivers that support *both* DT
and DSD, there may be exceptions to the rule where we have to do
things
a bit differently between DT and ACPI based systems (i.e. mobile and
enterprise).
DSD is *not* officially a method for getting DT properties into
ACPI,
though it may be used in that way in some cases (like the device
property above). In this particular case, I think it is preferred.
Generally speaking, the use of DT properties via DSD should use the DT
properties exactly, with a documented (sic) set of transformations for
known ACPI defined objects, like GPIO lines. These will require a
compatible property and will use the PRP0001 HID. These usages need not
be defined in the DSD database.
The other use for DSD is ACPI (or PCI, etc.) enumerated devices which
have a unique hardware ID. For these, the property sets should be
defined in the DSD database. While there is nothing preventing a vendor
from reusing DT property names, it was not the intent of the database
to map DT Schemas to subtly different ACPI DSD property sets with a new
set of HIDs. Some DT properties are Linux-specific (keycodes for
examples) and are not appropriate for an OS agnostic property
definition.
It isn't clear to me yet what the right thing is for this particular
device is.
Is there an intent to make this device available using these properties
in OSes besides Linux? If so, a new HID is appropriate, but I do think
we need to look more closely at src-clock-hz to determine how we will
handle this sort of thing in the general case.
Would like to hear from Rafael on this.
>
>
> Consider the 'src-clock-hz' property proposed here. That has no
> need to
> exist in a DT binding, but we have no other mechanism to acquire
> the
> relevant information in ACPI.
This is a fixed clock frequency set by boot firmware, and it does not
change at run-time. So what extra value do we get by acquiring this
value via clk_get_rate() instead of directly acquiring it from
DT/DSD?
This way, we get a unified solution for both DT and ACPI based
systems.
I agree in some cases it makes perfect sense for the ACPI based
solution
to diverge from the DT based solution, however I don't see a need or
value in doing that here.
In general, if the information is discoverable programmatically, we
should not be hardcoding it someplace. DSD is intended to describe
hardware that cannot be discovered otherwise.
BTW...Agree with your earlier note that we can change the name of
this
particular property. Let us know what you prefer this property to be
called.
>
>
> There are going to be necessary idiomatic differences.
>
> Thanks,
> Mark.
>
Thanks,
Harb
--
Darren Hart
Intel Open Source Technology Center