| Message ID | 20230310190634.5053-3-dipenp@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Tegra234 HTE support | expand |
On 10/03/2023 20:06, Dipen Patel wrote: > Added timestamp provider support for the Tegra234 in devicetree > bindings. In addition, it addresses review comments from the > previous review round as follows: > - Removes nvidia,slices property. This was not necessary as it > is a constant value and can be hardcoded inside the driver code. > - Adds nvidia,gpio-controller property. This simplifies how GTE driver > retrieves GPIO controller instance, see below explanation. > > Without this property code would look like: > if (of_device_is_compatible(dev->of_node, "nvidia,tegra194-gte-aon")) > hte_dev->c = gpiochip_find("tegra194-gpio-aon", > tegra_get_gpiochip_from_name); > else if (of_device_is_compatible(dev->of_node, "nvidia,tegra234-gte-aon")) > hte_dev->c = gpiochip_find("tegra234-gpio-aon", > tegra_get_gpiochip_from_name); > else > return -ENODEV; > > This means for every future addition of the compatible string, if else > condition statements have to be expanded. > > With the property: > gpio_ctrl = of_parse_phandle(dev->of_node, "nvidia,gpio-controller", 0); > .... > hte_dev->c = gpiochip_find(gpio_ctrl, tegra_get_gpiochip_from_of_node); > > We haven't technically started making use of these bindings, so > backwards-compatibility shouldn't be an issue yet. Unfortunately, I don't understand this statement. The nvidia,tegra194-gte-aon with removed property is in a released kernel v6.2. What does it mean "technically"? It's a released kernel thus it is a released ABI. And since DTS always go to separate branch, your patch #4 breaks existing DTS (return -ENODEV;) - it is not bisectable. > > Signed-off-by: Dipen Patel <dipenp@nvidia.com> > --- Best regards, Krzysztof
On 3/12/23 8:47 AM, Krzysztof Kozlowski wrote: > On 10/03/2023 20:06, Dipen Patel wrote: >> Added timestamp provider support for the Tegra234 in devicetree >> bindings. In addition, it addresses review comments from the >> previous review round as follows: >> - Removes nvidia,slices property. This was not necessary as it >> is a constant value and can be hardcoded inside the driver code. >> - Adds nvidia,gpio-controller property. This simplifies how GTE driver >> retrieves GPIO controller instance, see below explanation. >> >> Without this property code would look like: >> if (of_device_is_compatible(dev->of_node, "nvidia,tegra194-gte-aon")) >> hte_dev->c = gpiochip_find("tegra194-gpio-aon", >> tegra_get_gpiochip_from_name); >> else if (of_device_is_compatible(dev->of_node, "nvidia,tegra234-gte-aon")) >> hte_dev->c = gpiochip_find("tegra234-gpio-aon", >> tegra_get_gpiochip_from_name); >> else >> return -ENODEV; >> >> This means for every future addition of the compatible string, if else >> condition statements have to be expanded. >> >> With the property: >> gpio_ctrl = of_parse_phandle(dev->of_node, "nvidia,gpio-controller", 0); >> .... >> hte_dev->c = gpiochip_find(gpio_ctrl, tegra_get_gpiochip_from_of_node); >> >> We haven't technically started making use of these bindings, so >> backwards-compatibility shouldn't be an issue yet. > > Unfortunately, I don't understand this statement. The > nvidia,tegra194-gte-aon with removed property is in a released kernel > v6.2. What does it mean "technically"? It's a released kernel thus it is > a released ABI. There is no active user of that driver, so even if it breaks 6.2, it is fine as there is no one to complain about it. > > And since DTS always go to separate branch, your patch #4 breaks > existing DTS (return -ENODEV;) - it is not bisectable. > >> >> Signed-off-by: Dipen Patel <dipenp@nvidia.com> >> --- > > > Best regards, > Krzysztof >
On 13/03/2023 18:05, Dipen Patel wrote: > On 3/12/23 8:47 AM, Krzysztof Kozlowski wrote: >> On 10/03/2023 20:06, Dipen Patel wrote: >>> Added timestamp provider support for the Tegra234 in devicetree >>> bindings. In addition, it addresses review comments from the >>> previous review round as follows: >>> - Removes nvidia,slices property. This was not necessary as it >>> is a constant value and can be hardcoded inside the driver code. >>> - Adds nvidia,gpio-controller property. This simplifies how GTE driver >>> retrieves GPIO controller instance, see below explanation. >>> >>> Without this property code would look like: >>> if (of_device_is_compatible(dev->of_node, "nvidia,tegra194-gte-aon")) >>> hte_dev->c = gpiochip_find("tegra194-gpio-aon", >>> tegra_get_gpiochip_from_name); >>> else if (of_device_is_compatible(dev->of_node, "nvidia,tegra234-gte-aon")) >>> hte_dev->c = gpiochip_find("tegra234-gpio-aon", >>> tegra_get_gpiochip_from_name); >>> else >>> return -ENODEV; >>> >>> This means for every future addition of the compatible string, if else >>> condition statements have to be expanded. >>> >>> With the property: >>> gpio_ctrl = of_parse_phandle(dev->of_node, "nvidia,gpio-controller", 0); >>> .... >>> hte_dev->c = gpiochip_find(gpio_ctrl, tegra_get_gpiochip_from_of_node); >>> >>> We haven't technically started making use of these bindings, so >>> backwards-compatibility shouldn't be an issue yet. >> >> Unfortunately, I don't understand this statement. The >> nvidia,tegra194-gte-aon with removed property is in a released kernel >> v6.2. What does it mean "technically"? It's a released kernel thus it is >> a released ABI. > > There is no active user of that driver, so even if it breaks 6.2, it is fine > as there is no one to complain about it. How do you know? It's a released kernel, thus how can you ask millions of people if they use it or not? Best regards, Krzysztof
On 3/13/23 10:55 AM, Krzysztof Kozlowski wrote: > On 13/03/2023 18:05, Dipen Patel wrote: >> On 3/12/23 8:47 AM, Krzysztof Kozlowski wrote: >>> On 10/03/2023 20:06, Dipen Patel wrote: >>>> Added timestamp provider support for the Tegra234 in devicetree >>>> bindings. In addition, it addresses review comments from the >>>> previous review round as follows: >>>> - Removes nvidia,slices property. This was not necessary as it >>>> is a constant value and can be hardcoded inside the driver code. >>>> - Adds nvidia,gpio-controller property. This simplifies how GTE driver >>>> retrieves GPIO controller instance, see below explanation. >>>> >>>> Without this property code would look like: >>>> if (of_device_is_compatible(dev->of_node, "nvidia,tegra194-gte-aon")) >>>> hte_dev->c = gpiochip_find("tegra194-gpio-aon", >>>> tegra_get_gpiochip_from_name); >>>> else if (of_device_is_compatible(dev->of_node, "nvidia,tegra234-gte-aon")) >>>> hte_dev->c = gpiochip_find("tegra234-gpio-aon", >>>> tegra_get_gpiochip_from_name); >>>> else >>>> return -ENODEV; >>>> >>>> This means for every future addition of the compatible string, if else >>>> condition statements have to be expanded. >>>> >>>> With the property: >>>> gpio_ctrl = of_parse_phandle(dev->of_node, "nvidia,gpio-controller", 0); >>>> .... >>>> hte_dev->c = gpiochip_find(gpio_ctrl, tegra_get_gpiochip_from_of_node); >>>> >>>> We haven't technically started making use of these bindings, so >>>> backwards-compatibility shouldn't be an issue yet. >>> >>> Unfortunately, I don't understand this statement. The >>> nvidia,tegra194-gte-aon with removed property is in a released kernel >>> v6.2. What does it mean "technically"? It's a released kernel thus it is >>> a released ABI. >> >> There is no active user of that driver, so even if it breaks 6.2, it is fine >> as there is no one to complain about it. > > How do you know? It's a released kernel, thus how can you ask millions > of people if they use it or not? Please help me understand, if I am targeting these set of changes for the kernel 6.4, wouldn't all the patches land on v6.4 at the same time no matter the tree it will go from? Also, if user is at v6.2, how this will break as at that version, it will have the old bindings and old driver, right? > > Best regards, > Krzysztof >
Hi Dipen, thanks for maintaining HTE! On Fri, Mar 10, 2023 at 8:06 PM Dipen Patel <dipenp@nvidia.com> wrote: > - nvidia,slices: > - $ref: /schemas/types.yaml#/definitions/uint32 I would not delete this, just mark it deprecated. nvidia,slices: $ref: /schemas/types.yaml#/definitions/uint32 deprecated: true (And remove it from required, of course) This way you do not need to explain about why it was deleted, it's just deprecated, which is fine. Yours, Linus Walleij
On 3/13/23 2:57 PM, Linus Walleij wrote: > Hi Dipen, > > thanks for maintaining HTE! > > On Fri, Mar 10, 2023 at 8:06 PM Dipen Patel <dipenp@nvidia.com> wrote: > >> - nvidia,slices: >> - $ref: /schemas/types.yaml#/definitions/uint32 > > I would not delete this, just mark it deprecated. > > nvidia,slices: > $ref: /schemas/types.yaml#/definitions/uint32 > deprecated: true > > (And remove it from required, of course) > > This way you do not need to explain about why it was > deleted, it's just deprecated, which is fine. Great suggestion, thanks, will make changes in the next patch. > > Yours, > Linus Walleij
On 3/13/23 4:49 PM, Dipen Patel wrote: > On 3/13/23 2:57 PM, Linus Walleij wrote: >> Hi Dipen, >> >> thanks for maintaining HTE! >> >> On Fri, Mar 10, 2023 at 8:06 PM Dipen Patel <dipenp@nvidia.com> wrote: >> >>> - nvidia,slices: >>> - $ref: /schemas/types.yaml#/definitions/uint32 >> >> I would not delete this, just mark it deprecated. >> >> nvidia,slices: >> $ref: /schemas/types.yaml#/definitions/uint32 >> deprecated: true >> >> (And remove it from required, of course) >> >> This way you do not need to explain about why it was >> deleted, it's just deprecated, which is fine. > > Great suggestion, thanks, will make changes in the next patch. However, as I understood, current point of contention/discussion is addition of the nvidia,gpio-controller property. >> >> Yours, >> Linus Walleij >
On Tue, Mar 14, 2023 at 1:02 AM Dipen Patel <dipenp@nvidia.com> wrote: > However, as I understood, current point of contention/discussion is addition of the > nvidia,gpio-controller property. No I think you are talking past each other. Krzysztof talks about a "removed property": > Unfortunately, I don't understand this statement. The > nvidia,tegra194-gte-aon with removed property is in a released kernel > v6.2. What does it mean "technically"? It's a released kernel thus it is > a released ABI. The only property you remove is nvidia,slices, so deprecate it instead, problem solved. I don't think the added phandle is a problem, it can't cause backward compatibility issues since it is new. Yours, Linus Walleij
On 13/03/2023 22:49, Dipen Patel wrote: > On 3/13/23 10:55 AM, Krzysztof Kozlowski wrote: >> On 13/03/2023 18:05, Dipen Patel wrote: >>> On 3/12/23 8:47 AM, Krzysztof Kozlowski wrote: >>>> On 10/03/2023 20:06, Dipen Patel wrote: >>>>> Added timestamp provider support for the Tegra234 in devicetree >>>>> bindings. In addition, it addresses review comments from the >>>>> previous review round as follows: >>>>> - Removes nvidia,slices property. This was not necessary as it >>>>> is a constant value and can be hardcoded inside the driver code. >>>>> - Adds nvidia,gpio-controller property. This simplifies how GTE driver >>>>> retrieves GPIO controller instance, see below explanation. >>>>> >>>>> Without this property code would look like: >>>>> if (of_device_is_compatible(dev->of_node, "nvidia,tegra194-gte-aon")) >>>>> hte_dev->c = gpiochip_find("tegra194-gpio-aon", >>>>> tegra_get_gpiochip_from_name); >>>>> else if (of_device_is_compatible(dev->of_node, "nvidia,tegra234-gte-aon")) >>>>> hte_dev->c = gpiochip_find("tegra234-gpio-aon", >>>>> tegra_get_gpiochip_from_name); >>>>> else >>>>> return -ENODEV; >>>>> >>>>> This means for every future addition of the compatible string, if else >>>>> condition statements have to be expanded. >>>>> >>>>> With the property: >>>>> gpio_ctrl = of_parse_phandle(dev->of_node, "nvidia,gpio-controller", 0); >>>>> .... >>>>> hte_dev->c = gpiochip_find(gpio_ctrl, tegra_get_gpiochip_from_of_node); >>>>> >>>>> We haven't technically started making use of these bindings, so >>>>> backwards-compatibility shouldn't be an issue yet. >>>> >>>> Unfortunately, I don't understand this statement. The >>>> nvidia,tegra194-gte-aon with removed property is in a released kernel >>>> v6.2. What does it mean "technically"? It's a released kernel thus it is >>>> a released ABI. >>> >>> There is no active user of that driver, so even if it breaks 6.2, it is fine >>> as there is no one to complain about it. >> >> How do you know? It's a released kernel, thus how can you ask millions >> of people if they use it or not? > > Please help me understand, if I am targeting these set of changes for the kernel > 6.4, wouldn't all the patches land on v6.4 at the same time no matter the tree it No, that's not how we do things. Changes *must be bisectable* and *DTS always* goes to separate branch, so how do you ensure this in your current flow? I don't see it. The patch #4 should break the bisectability. > will go from? Also, if user is at v6.2, how this will break as at that version, it > will have the old bindings and old driver, right? Bindings define ABI. You defined them like this in v6.2 thus someone is using them: 1. In other systems, bootloaders, firmwares, SW. 2. via DTS written for v6.2 ABI. Newer kernel should not break existing DTS and we do not talk about in-kernel DTS, just like we do not talk about in-kernel user-space applications when using same argument for their compatibility. Best regards, Krzysztof
On 13/03/2023 23:49, Dipen Patel wrote: > On 3/13/23 2:57 PM, Linus Walleij wrote: >> Hi Dipen, >> >> thanks for maintaining HTE! >> >> On Fri, Mar 10, 2023 at 8:06 PM Dipen Patel <dipenp@nvidia.com> wrote: >> >>> - nvidia,slices: >>> - $ref: /schemas/types.yaml#/definitions/uint32 >> >> I would not delete this, just mark it deprecated. >> >> nvidia,slices: >> $ref: /schemas/types.yaml#/definitions/uint32 >> deprecated: true >> >> (And remove it from required, of course) >> >> This way you do not need to explain about why it was >> deleted, it's just deprecated, which is fine. > > Great suggestion, thanks, will make changes in the next patch. When you deprecate it, please make this a separate patch because it is separate from adding Tegra234 support. Similarly you will want to have a separate patch to remove support of the property from the driver as well. Jon
diff --git a/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml b/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml index c31e207d1652..eb904ac2f331 100644 --- a/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml +++ b/Documentation/devicetree/bindings/timestamp/nvidia,tegra194-hte.yaml @@ -4,7 +4,7 @@ $id: http://devicetree.org/schemas/timestamp/nvidia,tegra194-hte.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# -title: Tegra194 on chip generic hardware timestamping engine (HTE) +title: Tegra on chip generic hardware timestamping engine (HTE) provider maintainers: - Dipen Patel <dipenp@nvidia.com> @@ -23,6 +23,8 @@ properties: enum: - nvidia,tegra194-gte-aon - nvidia,tegra194-gte-lic + - nvidia,tegra234-gte-aon + - nvidia,tegra234-gte-lic reg: maxItems: 1 @@ -38,14 +40,11 @@ properties: minimum: 1 maximum: 256 - nvidia,slices: - $ref: /schemas/types.yaml#/definitions/uint32 + nvidia,gpio-controller: + $ref: /schemas/types.yaml#/definitions/phandle description: - HTE lines are arranged in 32 bit slice where each bit represents different - line/signal that it can enable/configure for the timestamp. It is u32 - property and depends on the HTE instance in the chip. The value 3 is for - GPIO GTE and 11 for IRQ GTE. - enum: [3, 11] + The phandle to AON gpio controller instance. This is required to handle + namespace conversion between GPIO and GTE. '#timestamp-cells': description: @@ -59,9 +58,20 @@ required: - compatible - reg - interrupts - - nvidia,slices - "#timestamp-cells" +allOf: + - if: + properties: + compatible: + contains: + enum: + - nvidia,tegra194-gte-aon + - nvidia,tegra234-gte-aon + then: + required: + - nvidia,gpio-controller + additionalProperties: false examples: @@ -71,7 +81,7 @@ examples: reg = <0xc1e0000 0x10000>; interrupts = <0 13 0x4>; nvidia,int-threshold = <1>; - nvidia,slices = <3>; + nvidia,gpio-controller = <&gpio_aon>; #timestamp-cells = <1>; }; @@ -81,7 +91,6 @@ examples: reg = <0x3aa0000 0x10000>; interrupts = <0 11 0x4>; nvidia,int-threshold = <1>; - nvidia,slices = <11>; #timestamp-cells = <1>; };
Added timestamp provider support for the Tegra234 in devicetree bindings. In addition, it addresses review comments from the previous review round as follows: - Removes nvidia,slices property. This was not necessary as it is a constant value and can be hardcoded inside the driver code. - Adds nvidia,gpio-controller property. This simplifies how GTE driver retrieves GPIO controller instance, see below explanation. Without this property code would look like: if (of_device_is_compatible(dev->of_node, "nvidia,tegra194-gte-aon")) hte_dev->c = gpiochip_find("tegra194-gpio-aon", tegra_get_gpiochip_from_name); else if (of_device_is_compatible(dev->of_node, "nvidia,tegra234-gte-aon")) hte_dev->c = gpiochip_find("tegra234-gpio-aon", tegra_get_gpiochip_from_name); else return -ENODEV; This means for every future addition of the compatible string, if else condition statements have to be expanded. With the property: gpio_ctrl = of_parse_phandle(dev->of_node, "nvidia,gpio-controller", 0); .... hte_dev->c = gpiochip_find(gpio_ctrl, tegra_get_gpiochip_from_of_node); We haven't technically started making use of these bindings, so backwards-compatibility shouldn't be an issue yet. Signed-off-by: Dipen Patel <dipenp@nvidia.com> --- v2: - Removed nvidia,slices property - Added nvidia,gpio-controller based on review comments from Thierry, this will help simplify the hte provider driver. v3: - Explained changes in detail in commit message - Added allOf section per review comment .../timestamp/nvidia,tegra194-hte.yaml | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-)