Re: [PATCH V3 2/2] dt-bindings: remoteproc: k3-dsp: Remove board-specific compatible from DT example

[Suman Anna]

Hi Sinthu,

On 9/24/21 12:25 PM, Suman Anna wrote:
> On 9/24/21 11:29 AM, Nishanth Menon wrote:
>> On 11:10-20210924, Suman Anna wrote:
>>> Hi Sinthu,
>>>
>>> On 9/17/21 4:54 AM, Sinthu Raja wrote:
>>>> From: Sinthu Raja <sinthu.raja@xxxxxx>
>>>>
>>>> The example includes a board-specific compatible property, this is
>>>> wrong as the example should be board agnostic and gets in the way of
>>>> additions for newer platforms. Replace the same with a generic soc
>>>> node.
>>>
>>> What board specific property? This description looks wrong.

Can you please repost dropping the Fixes line, and modifying the patch
description as follows:

dt-bindings: remoteproc: k3-dsp: Cleanup SoC compatible from DT example

The K3 DSP binding example used the root-node with a SoC compatible
property originally to address the dt_binding_check warnings resulting
from using a value of 2 for #address-cells and #size-cells as per most
common usage on K3 SoCs. Clean this up and replace it with a generic soc
node to keep it agnostic of the SoC or board compatibles that are outside
the scope of this binding.

With that,
Acked-by: Suman Anna <s-anna@xxxxxx>

Please update the R5F binding patch as well similarly. You can retain the
already received Acks.

regards
Suman


>>
>> See https://lore.kernel.org/all/1631794913.472895.1119414.nullmailer@xxxxxxxxxxxxxxxxxx/
>>
> 
> Yes, I understand you are now trying to add/scale for a board compatible and
> your patch is what triggered the warnings.
> 
> I see "ti,j721e" as an SoC compatible not board-specific.
> 
>>>
>>>>
>>>> Fixes: 2a2180206ab6 ("dt-bindings: remoteproc: Add bindings for C66x DSPs on TI K3 SoCs")
>>>
>>> What error are you trying to fix exactly? The example used below is actually how
>>> it exactly appears in the J721E dts files, and there are no errors with
>>> dt_binding_check.
>>
>> The rproc binding should have nothing to do with j721e SoC node
>> description. it should describe the rproc node that is described in
>> binding.
> 
> You can go back and look at my original dt-binding submissions and the reasons
> for me to add a root-cell. They are to suppress the warnings seen with using two
> address-cells in the DSP example nodes which use the actual node definitions
> from the J721E SoC.
> 
> v1:
> https://patchwork.kernel.org/project/linux-remoteproc/patch/20200325201839.15896-2-s-anna@xxxxxx/
> v2:
> https://patchwork.kernel.org/project/linux-remoteproc/patch/20200521001006.2725-3-s-anna@xxxxxx/
> v3:
> https://patchwork.kernel.org/project/linux-remoteproc/patch/20200612224914.7634-5-s-anna@xxxxxx/
> 
>>
>>>
>>> This is more a cleanup than a fix.  You can look through the original binding
>>> submission patches to see why it is done like this.
>>
>> This is blocking any updates we would want to do in k3.yaml.
> 
> One other way would have been to just add the new enforced compatible (since you
> are actually changing the k3.yaml binding and diverging from what was there
> before) here along with your updates, if you didn't want to add it in the
> previous compatible way.
> 
> FWIW, there are no dt_binding_check errors on this binding before your
> modifications, that's why I am asking what is the "Fixes" with the original patch.
> 
>>>
>>> If this is triggered by the changes you are making to k3.yaml file as part of
>>> the J721E EAIK changes, then you probably may want to look at how you are doing
>>
>>> that again. Looks like the k3.yaml file is being modified now to enforce
>>> "board-compatible", "soc-compatible" which may have triggered an error in this file.
>>>
>>> Please evaluate if you need to modify it to support just the "soc-compatible" as
>>> one of the items.
>>
>> See above link. This is not to do with eaik / sk. I am trying to
>> standardize the board definitions in yaml for k3 and this binding
>> specifically is getting in the way.
> 
> Yeah finally. I remember I had asked you about why we are doing it differently
> between AM65x and J721E/J7200. +1 for the direction.
> 
>>
>>
>> I still don't understand what your contention is here. Are you arguing
>> that the binding example is correct and should be tied to a platform?
> 
> I am not saying it should be tied to a platform, but I have used the example as
> it appears on J721E SoCs. I am commenting that it is not a "Fixes:" and the
> patch description needs updates.
> 
>>
>>
>> Yes, I know I can introduce oneOf and a little more intricate solution,
>> 	but besides that, i disagree that a rproc binding should even
>> 	have SoC specific top level node description in it.
> 
> Please see the reasoning in the original submissions. I could not use 2
> address-cells and size-cells without the top-level node additions, and I didn't
> want to use bogus examples.
> 
> Yes, the intricate solution would not have triggered the warning in this
> example, but your current change is also breaking your previous compatibility. I
> understand that the reality is always actually a "board-compatible",
> "soc-compatible", but as per your previous k3.yaml definition, all one needed
> was just a "ti,j721e" compatible in their dts files. Changing it now and calling
> the usage in this example "wrong" is not right either IMO.
> 
> 
>> a) rproc.yaml does'nt even describe the SoC. soc.yaml does.
>> b) The node property examples are supposed to be examples not tied to a
>>    specific SoC.
> 
> I would rather not use a completely bogus example since it is not very useful
> for customers trying to understand the binding. My philosophy has always been to
> define an example as it appears on an actual SoC so that it is easier for
> customers to comprehend the binding and example while comparing it to actual dts
> nodes.
> 
> regards
> Suman
> 
>>
>>>> Signed-off-by: Sinthu Raja <sinthu.raja@xxxxxx>
>>>> ---
>>>>
>>>> Changes since V2:
>>>> * review comment updates, including simplifying the changes, commit
>>>>   message and $subject updates.
>>>>
>>>> V2: https://lore.kernel.org/all/20210818074030.1877-1-sinthu.raja@xxxxxx/
>>>> V1: https://lore.kernel.org/all/20210817152005.21575-1-sinthu.raja@xxxxxx/
>>>>
>>>>  .../devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml       | 4 +---
>>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml
>>>> index 6070456a7b67..5ec6505ac408 100644
>>>> --- a/Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml
>>>> +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-dsp-rproc.yaml
>>>> @@ -133,9 +133,7 @@ unevaluatedProperties: false
>>>>  
>>>>  examples:
>>>>    - |
>>>> -    / {
>>>> -        model = "Texas Instruments K3 J721E SoC";
>>>> -        compatible = "ti,j721e";
>>>> +    soc {
>>>
>>> While this may be resolving the dt_bindings_check you might be seeing with the
>>> modified k3.yaml, note that "soc" property is not used on K3 dts files, you
>>> might be creating confusion for people who look at this example and the actual
>>> usage.
>>
>>
>> It is a common usage model. NOTE: these are example nodes and NOT meant
>> as SoC representation. I dont see the confusion.
>>
> 
> 
> 
>