diff mbox series

[1/4] dt-bindings: memory: additional compatible strings for Broadcom DPFE

Message ID 20231205184741.3092376-2-mmayer@broadcom.com
State New
Headers
Series memory: brcmstb_dpfe: support DPFE API v4 |

Commit Message

Markus Mayer Dec. 5, 2023, 6:47 p.m. UTC 
  Add versioned compatible strings for Broadcom DPFE. These take the form
brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.

These API version related compatible strings are more specific than the
catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
strings.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 .../bindings/memory-controllers/brcm,dpfe-cpu.yaml        | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Krzysztof Kozlowski Dec. 6, 2023, 11:09 a.m. UTC | #1 
On 05/12/2023 19:47, Markus Mayer wrote:
> Add versioned compatible strings for Broadcom DPFE. These take the form
> brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.
> 
> These API version related compatible strings are more specific than the
> catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
> strings.

None of this explains: Why? I don't see any point in this and commit
does not explain.

> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  .../bindings/memory-controllers/brcm,dpfe-cpu.yaml        | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> index 08cbdcddfead..6dffa7b62baf 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> @@ -16,6 +16,11 @@ properties:
>        - enum:
>            - brcm,bcm7271-dpfe-cpu
>            - brcm,bcm7268-dpfe-cpu
> +      - enum:
> +          - brcm,dpfe-cpu-v1
> +          - brcm,dpfe-cpu-v2
> +          - brcm,dpfe-cpu-v3
> +          - brcm,dpfe-cpu-v4

No, that's just wrong. So you want to say bcm7271 is brcm,dpfe-cpu-v4?

Best regards,
Krzysztof
Florian Fainelli Dec. 6, 2023, 4:32 p.m. UTC | #2 
On 12/6/2023 3:09 AM, Krzysztof Kozlowski wrote:
> On 05/12/2023 19:47, Markus Mayer wrote:
>> Add versioned compatible strings for Broadcom DPFE. These take the form
>> brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.
>>
>> These API version related compatible strings are more specific than the
>> catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
>> strings.
> 
> None of this explains: Why? I don't see any point in this and commit
> does not explain.
> 
>>
>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> ---
>>   .../bindings/memory-controllers/brcm,dpfe-cpu.yaml        | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>> index 08cbdcddfead..6dffa7b62baf 100644
>> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>> @@ -16,6 +16,11 @@ properties:
>>         - enum:
>>             - brcm,bcm7271-dpfe-cpu
>>             - brcm,bcm7268-dpfe-cpu
>> +      - enum:
>> +          - brcm,dpfe-cpu-v1
>> +          - brcm,dpfe-cpu-v2
>> +          - brcm,dpfe-cpu-v3
>> +          - brcm,dpfe-cpu-v4
> 
> No, that's just wrong. So you want to say bcm7271 is brcm,dpfe-cpu-v4?

No as the example shows it "speaks" API v1.

I would be inclined to completely remove the chip specific compatible 
strings from the binding because they are not sufficient or descriptive 
enough to determine which API version is being spoken, since the 
firmware is unfortunately allowed to change major APIs (and the 
messaging format, because why not?) at a moments notice.
Krzysztof Kozlowski Dec. 6, 2023, 5:29 p.m. UTC | #3 
On 06/12/2023 17:32, Florian Fainelli wrote:
> 
> 
> On 12/6/2023 3:09 AM, Krzysztof Kozlowski wrote:
>> On 05/12/2023 19:47, Markus Mayer wrote:
>>> Add versioned compatible strings for Broadcom DPFE. These take the form
>>> brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.
>>>
>>> These API version related compatible strings are more specific than the
>>> catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
>>> strings.
>>
>> None of this explains: Why? I don't see any point in this and commit
>> does not explain.
>>
>>>
>>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>>> ---
>>>   .../bindings/memory-controllers/brcm,dpfe-cpu.yaml        | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>> index 08cbdcddfead..6dffa7b62baf 100644
>>> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>> @@ -16,6 +16,11 @@ properties:
>>>         - enum:
>>>             - brcm,bcm7271-dpfe-cpu
>>>             - brcm,bcm7268-dpfe-cpu
>>> +      - enum:
>>> +          - brcm,dpfe-cpu-v1
>>> +          - brcm,dpfe-cpu-v2
>>> +          - brcm,dpfe-cpu-v3
>>> +          - brcm,dpfe-cpu-v4
>>
>> No, that's just wrong. So you want to say bcm7271 is brcm,dpfe-cpu-v4?
> 
> No as the example shows it "speaks" API v1.

Example is not a binding. It does not matter except of validating the
binding. This is just incorrect.

> 
> I would be inclined to completely remove the chip specific compatible 
> strings from the binding because they are not sufficient or descriptive 
> enough to determine which API version is being spoken, since the 
> firmware is unfortunately allowed to change major APIs (and the 
> messaging format, because why not?) at a moments notice.

Then versions do not give you anything more.

Best regards,
Krzysztof
Florian Fainelli Dec. 6, 2023, 5:36 p.m. UTC | #4 
On 12/6/23 09:29, Krzysztof Kozlowski wrote:
> On 06/12/2023 17:32, Florian Fainelli wrote:
>>
>>
>> On 12/6/2023 3:09 AM, Krzysztof Kozlowski wrote:
>>> On 05/12/2023 19:47, Markus Mayer wrote:
>>>> Add versioned compatible strings for Broadcom DPFE. These take the form
>>>> brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.
>>>>
>>>> These API version related compatible strings are more specific than the
>>>> catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
>>>> strings.
>>>
>>> None of this explains: Why? I don't see any point in this and commit
>>> does not explain.
>>>
>>>>
>>>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>>>> ---
>>>>    .../bindings/memory-controllers/brcm,dpfe-cpu.yaml        | 8 +++++++-
>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>> index 08cbdcddfead..6dffa7b62baf 100644
>>>> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>> @@ -16,6 +16,11 @@ properties:
>>>>          - enum:
>>>>              - brcm,bcm7271-dpfe-cpu
>>>>              - brcm,bcm7268-dpfe-cpu
>>>> +      - enum:
>>>> +          - brcm,dpfe-cpu-v1
>>>> +          - brcm,dpfe-cpu-v2
>>>> +          - brcm,dpfe-cpu-v3
>>>> +          - brcm,dpfe-cpu-v4
>>>
>>> No, that's just wrong. So you want to say bcm7271 is brcm,dpfe-cpu-v4?
>>
>> No as the example shows it "speaks" API v1.
> 
> Example is not a binding. It does not matter except of validating the
> binding. This is just incorrect.
> 
>>
>> I would be inclined to completely remove the chip specific compatible
>> strings from the binding because they are not sufficient or descriptive
>> enough to determine which API version is being spoken, since the
>> firmware is unfortunately allowed to change major APIs (and the
>> messaging format, because why not?) at a moments notice.
> 
> Then versions do not give you anything more.

The versions indicate exactly which API to be spoken to with the 
firmware. The firmware API was not properly designed, it should have had 
a way to indicate which API it has, regardless of the messaging format 
it implements, but for reasons unknown that is not how it was implemented.

Essentially we need to know right away and ahead of time which API to be 
used, otherwise that means doing runtime detection like what patch 4 
does which you do not want to see.
Krzysztof Kozlowski Dec. 6, 2023, 5:42 p.m. UTC | #5 
On 06/12/2023 18:36, Florian Fainelli wrote:
> On 12/6/23 09:29, Krzysztof Kozlowski wrote:
>> On 06/12/2023 17:32, Florian Fainelli wrote:
>>>
>>>
>>> On 12/6/2023 3:09 AM, Krzysztof Kozlowski wrote:
>>>> On 05/12/2023 19:47, Markus Mayer wrote:
>>>>> Add versioned compatible strings for Broadcom DPFE. These take the form
>>>>> brcm,dpfe-cpu-v<N> where <N> is a number from 1 to 4.
>>>>>
>>>>> These API version related compatible strings are more specific than the
>>>>> catch-all "brcm,dpfe-cpu" and more generic than chip-specific compatible
>>>>> strings.
>>>>
>>>> None of this explains: Why? I don't see any point in this and commit
>>>> does not explain.
>>>>
>>>>>
>>>>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>>>>> ---
>>>>>    .../bindings/memory-controllers/brcm,dpfe-cpu.yaml        | 8 +++++++-
>>>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>>> index 08cbdcddfead..6dffa7b62baf 100644
>>>>> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>>> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>>>> @@ -16,6 +16,11 @@ properties:
>>>>>          - enum:
>>>>>              - brcm,bcm7271-dpfe-cpu
>>>>>              - brcm,bcm7268-dpfe-cpu
>>>>> +      - enum:
>>>>> +          - brcm,dpfe-cpu-v1
>>>>> +          - brcm,dpfe-cpu-v2
>>>>> +          - brcm,dpfe-cpu-v3
>>>>> +          - brcm,dpfe-cpu-v4
>>>>
>>>> No, that's just wrong. So you want to say bcm7271 is brcm,dpfe-cpu-v4?
>>>
>>> No as the example shows it "speaks" API v1.
>>
>> Example is not a binding. It does not matter except of validating the
>> binding. This is just incorrect.
>>
>>>
>>> I would be inclined to completely remove the chip specific compatible
>>> strings from the binding because they are not sufficient or descriptive
>>> enough to determine which API version is being spoken, since the
>>> firmware is unfortunately allowed to change major APIs (and the
>>> messaging format, because why not?) at a moments notice.
>>
>> Then versions do not give you anything more.
> 
> The versions indicate exactly which API to be spoken to with the 
> firmware. The firmware API was not properly designed, it should have had 
> a way to indicate which API it has, regardless of the messaging format 
> it implements, but for reasons unknown that is not how it was implemented.
> 
> Essentially we need to know right away and ahead of time which API to be 
> used, otherwise that means doing runtime detection like what patch 4 
> does which you do not want to see.

Yeah, I see, you explained this deeper in response to 3/4, which I read
after this one.

Deprecating specific compatibles makes sense. If you have subset of FW
per given SoC, you could keep the specific compatible followed by subset
of version-compatibles (e.g. bcm7271 + v1 + generic fallback). However
then generic fallback is useless and you should actually drop it. The
only, *ONLY* point of generic fallback is to be used by OS alone. In
that case it cannot be used alone, so it is useless.

We do not use generic compatibles in a way of "I want to call all of
these devices a DPFE" or "I want to call it a default".

Now, if you do not have subset of FW per given SoC, so anything can
match with anything, then in one commit:
1. Deprecate specific compatible followed by useless generic fallback
2. Add versioned-compatibles alone, since generic fallback gives nothing.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
index 08cbdcddfead..6dffa7b62baf 100644
--- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
@@ -16,6 +16,11 @@  properties:
       - enum:
           - brcm,bcm7271-dpfe-cpu
           - brcm,bcm7268-dpfe-cpu
+      - enum:
+          - brcm,dpfe-cpu-v1
+          - brcm,dpfe-cpu-v2
+          - brcm,dpfe-cpu-v3
+          - brcm,dpfe-cpu-v4
       - const: brcm,dpfe-cpu
 
   reg:
@@ -40,7 +45,8 @@  additionalProperties: false
 examples:
   - |
     dpfe-cpu@f1132000 {
-        compatible = "brcm,bcm7271-dpfe-cpu", "brcm,dpfe-cpu";
+        compatible = "brcm,bcm7271-dpfe-cpu", "brcm,dpfe-cpu-v1",
+              "brcm,dpfe-cpu";
         reg = <0xf1132000 0x180>,
               <0xf1134000 0x1000>,
               <0xf1138000 0x4000>;