[V3,2/6] dt-bindings: timestamp: Add Tegra234 support

  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(-)
  

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