From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Tue, 7 Apr 2020 20:58:13 -0600 Subject: [PATCH v3 13/29] dts: Add a binding for hid-over-i2c In-Reply-To: References: <20200330231305.130488-1-sjg@chromium.org> <20200330231305.130488-2-sjg@chromium.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Wolfgang, On Tue, 31 Mar 2020 at 13:25, Wolfgang Wallner wrote: > > Hi Simon, > > -----"Simon Glass" schrieb: ----- > > >An: u-boot at lists.denx.de > >Von: "Simon Glass" > >Datum: 31.03.2020 01:14 > >Kopie: "Andy Shevchenko" , > >"Wolfgang Wallner" , "Leif > >Lindholm" , "Simon Glass" > >Betreff: [PATCH v3 14/29] acpi: Add a binding for ACPI settings in > >the device tree > > > > Devices need to report various identifiers in the ACPI tables. Rather than > > hard-coding these in drivers it is typically better to put them in the > > device tree. > > > > Add a binding file to describe this. > > > > Signed-off-by: Simon Glass > > --- > > > > Changes in v3: > > - Add a pointer to information about acpi,compatible > > - Change the example to ELAN > > - Correct description of acpi,probed > > - Drop hid-descr-addr > > - Drop mention of PRIC > > I understand now where the term "PRIC" came from. > The property "acpi,has-power-resource" triggers the function > acpi_device_add_power_res(), which writes a PowerResource ('PowerResource' > as specified in section 7.2 of ACPI 6.3). This function comes from Coreboot, > and that implementation uses "PRIC" as the hardcoded device name. That's it, > it is an arbitrary chosen device name (probably meaning "power IC" ... ?). > > Anyway, dropping the term "PRIC" makes the description easier to understand. > The important information is that "acpi,has-power-resource" leads to the > addition of a PowerResource entry. > > > - Just add the device.txt binding file in this patch > > - Rename acpi,desc to acpi,ddn > > > > Changes in v2: > > - Add the hid-over-i2c binding document > > - Fix definition of HID > > - Infer hid-over-i2c CID value > > > > doc/device-tree-bindings/device.txt | 37 +++++++++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > create mode 100644 doc/device-tree-bindings/device.txt > > > > diff --git a/doc/device-tree-bindings/device.txt b/doc/device-tree-bindings/device.txt > > new file mode 100644 > > index 00000000000..06c2b84b6d5 > > --- /dev/null > > +++ b/doc/device-tree-bindings/device.txt > > @@ -0,0 +1,37 @@ > > +Devices > > +======= > > + > > +Device bindings are described by their own individual binding files. > > + > > +U-Boot provides for some optional properties which are documented here. See > > +also hid-over-i2c.txt which describes HID devices. See also > > +Documentation/firmware-guide/acpi/enumeration.rst in the Linux kernel for > > +the acpi,compatible property. > > + > > + - acpi,has-power-resource : (boolean) true if this device has a power resource. > > + This causes an ACPI PowerResource to be written containing the properties > > + provided by this binding, to describe how to handle powering the device up > > + and down using GPIOs > > + - acpi,compatible : compatible string to report > > I still don't see a use case for a new "acpi,compatible" property. > I will take a step back, and explain my understanding of the involved pieces > and why I think adding "acpi,compatible" is of no benefit. > > Summary: > As far as I understand the proposed "acpi, compatible" property, the following > would happen: > We have "acpi,compatible" in Devicetree, which results in a "compatible"-entry > in ACPI's _DSD method, which is then again interpreted as the > "compatible"-property of Devicetree. IMHO it would be strange for "compatible" > and "acpi,compatible" to be different, so we could drop "acpi,compatible" and > use the existing "compatible" instead. But the compatible string is "hid-over-i2c". We don't want to have lots of different compatible strings here, different drivers which do the same thing. If we end up wanting a touchscreen drivers in U-Boot that might change, but for now I think a generic driver is easier. > > The _DSD-method for "PRP0001"-devices in ACPI allows to use Devicetree > properties inside ACPI, especially it allows to re-use Devicetree's > "compatible"-property. But this is for a different use case (using Devicetree > properties inside ACPI, not add ACPI properties in Devicetree). > > Detailed explanation: > > 1) ACPI Constraint: Devices need to have either _HID or _ADR > > ACPI puts constraints on what properties a device ("device" here means a > "Device()" entry in ASL code (DSDT or SSDT)) has to have. It has to have > either an _HID or an _ADR property depending on whether it is on an > enumerable bus: > > * if it is on an enumerable bus, it has to have an _ADR (address) > property (e.g. a PCI device on a PCI bus) > * if it is not on an enumerable bus, it has to have a _HID (hardware ID) > property (e.g. the GPIO controllers on the Apollo Lake SoC). The legal > values for _HID are specified and allow the type of the device to be > identified (a similar concept as the "compatible" property in devicetree) > > These contraints are specified in section 6.1 of ACPI 6.3 [1]. > > 2) ACPI's _DSD Method > > The Device Specific Data (_DSD) method provides a way to provide additional > device properties to device drivers. It returns a package of "Device Data > Descriptors", each consisting of a UUID and a package in a format defined > by the provided UUID. > > The details are specified in section 6.2.5 of ACPI 6.3 [1]. > > 3) Special UUID value for _DSD: daffd814-... > > The document "Device Properties UUID For _DSD" [2] specifies a special UUID > value: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 > > When this UUID is used in the package returned by in a _DSD method, > it means the format of the package after the UUID consits of packages > each of size 2. > The first entry in these packages always has to be a string, the second > entry can be an integer, a string, a reference or again a package. The > value of the string defines the type and allowed values of the second entry. > > The typical use case for this UUID is to return some kind of > key/value-pairs. > > The specification in [2] does not specify which strings are legal as > key names. This depends on the _HID of the device that implements the > _DSD method. > > 4) Special _HID value: PRP0001 > > When the _HID property has the value "PRP0001", the _DSD method is expected > to provide data with the "daffd814"-UUID which contains a "compatible" > property. > > This interpreted similar to the "compatible" property known from Devicetree. > > 5) Linux device-property API > > Linux provides a "device-property" API (define in include/linux/property.h) > which can be used instead of a Devicetree specific API. > E.g. using device_property_read_u32() instead of of_property_read_u32(). > Thank you very much for digging into this. I read the ACPI spec years ago but clearly need to read it again. > Putting these pieces together: > > Suppose the following use case: > There is an existing driver for Devicetree, but it should be used under > x86, where devices are usually described via ACPI. > > To avoid having to register a new _HID that would have the exact same > meaning in ACPI as an already existing Devicetree "compatible" string, the > possibilities desbribed by 2-4) allow to solve this use case while > respecting the contraints described in 1). > > Additionally, using the API described in 5) allows to add other Devicetree > properties (not only the "compatible" property) to the ACPI description of > the device. This allows to have a single device driver that can get its > device description from either Devicetree or ACPI, and the description is > basically the same in both worlds. This is described e.g. in the > presentation in [4]. > > [1] https://uefi.org/sites/default/files/resources/ACPI_6_3_final_Jan30.pdf > [2] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf > [3] Documentation/firmware-guide/acpi/enumeration.rst in the Linux kernel > [4] https://elinux.org/images/2/2d/Device_tree_acpi_compatibility-david_woodhouse-kernel_recipes_2015.pdf > > > + - acpi,ddn : Contains the string to use as the _DDN (DOS (Disk Operating > > + System) Device Name) > > + - acpi,hid : Contains the string to use as the HID (Hardware ID) > > + identifier _HID > > + - acpi,probed : Tells U-Boot to add 'linux,probed' to the ACPI tables so that > > + Linux will only load the driver if the device can be detected (e.g. on I2C > > + bus) > > Will support for 'linux,probed' be mainlined? Otherwise the description > should IMHO mention that it is an out-of-tree feature. OK > > > + - acpi,uid : _UID value for device > > + > > + > > +Example > > +------- > > + > > +elan_touchscreen: elan-touchscreen at 10 { > > + compatible = "i2c-chip"; > > Why would you use a generic compatible string in this case? > According to drivers/input/touchscreen/elants_i2c.c in the Linux kernel > the ACPI _HID "ELAN0001" refers to the same device as the Devicetree > compatible string "elan,ekth3500". > > Again, because of the direct relationship between "compatible" string and > _HID I think we could move that knowledge into a (stub-) driver for > "elan,ekth3500" and then we could avoid the need for "acpi,hid" here. But then we need a driver for the ELAN touchscreen. My goal here is to have all of these devices serviced by a single driver, using suitable properties in the DT. > > > + reg = <0x10>; > > + acpi,hid = "ELAN0001"; > > + acpi,ddn = "ELAN Touchscreen"; > > + interrupts-extended = <&acpi_gpe GPIO_21_IRQ > > + IRQ_TYPE_EDGE_FALLING>; > > + acpi,probed; > > +}; > > -- > > 2.26.0.rc2.310.g2932bb562d-goog > Regards, SImon