On 9/13/2016 7:01 AM, Graeme Gregory wrote: The issue here is that this is the name of these properties has already been defined for device tree. Austin is simply trying to leverage existing device properties in DSD. Please refer to the 4.7 kernel: drivers/i2c/busses/i2c-qup.c (line 1377) Had this been a new driver, we would have more control over the property naming. Unfortunately, changing the name at this point will have broader impacts, and may not be feasible/practical. -- Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Mark Rutland wrote: 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. 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. -- Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.

On 9/20/2016 1:51 PM, Mark Rutland wrote: 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. :) 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. 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. 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. 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. 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. Thanks, Harb -- Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

On Thu, 2016-09-22 at 17:51 +0100, Mark Brown wrote: I understand this to be the case for most properties, but there are at least some examples of things which are Linux specific (the only one I have experience with are the key codes for the gpio-keys and gpio-keys- polled drivers). Maybe something similar with LEDs and triggers? Maybe this is just corner case? -- Darren Hart Intel Open Source Technology Center

On Thu, Sep 22, 2016 at 12:39:38PM -0400, Abdulhamid, Harb wrote: The device_property_read_xxx() functions cater for the premise that key:value pairs are identical across DT and DSD, which is not the case in general. As Mark Brown said, DT is also intended to be OS neutral (and there are non-Linux users). Admittedly, by far the largest user base today is Linux, but that is also true for DSD. To be clear, I agree that this will always be a case-by-case issue. This is precisely why I claim above that device_property_read_xxx() is a false abstraction. Regardless of DSD, it's too late to get rid of the existing DT binding. The code already uses clk_get_rate() for DT, and for compatibility with existing DTBs will always have to. In this case, I don't have a problem with clock-frequency for the I2C bus frequency. Using 'source-clock-frequency' sounds reasonable. If the clock input (i.e. the connector on the I2C block, not the clock itself) has a name, then '${NAME}-clock-frequency' would probably be a better choice. Thanks, Mark.

On Thu, 2016-09-22 at 12:39 -0400, Abdulhamid, Harb wrote: Yes. And Yes. We need to have the discussion. Appreciate everyone's input. 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. 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. 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. -- Darren Hart Intel Open Source Technology Center

On 09/20/2016 11:34 AM, Mark Rutland wrote: My apologies for the delay in replying; I took two weeks off and am just now getting caught up on things... When using ACPI on arm64, the kernel documentation says, too, that the kernel must be able to assume that all PHYs and clocks have been setup properly by UEFI before control is handed to the kernel. So the question in my mind is: why do we need to know these clock values at all? What is the driver doing with them? -- ciao, al ----------------------------------- Al Stone Software Engineer Red Hat, Inc. ahs3(a)redhat.com -----------------------------------