[PATCH v2 0/5] memory: brcmstb_dpfe: support DPFE API v4

[PATCH v2 0/5] memory: brcmstb_dpfe: support DPFE API v4

[Markus Mayer]

It has become necessary to distinguish between the various DPFE API
versions by version number. Having just chip-specific compatible strings
no longer meeting our needs. Furthermore, the generic compatible string
"brcm,dpfe-cpu" is being removed as it doesn't serve any actual purpose.
Finally, a new DPFE API version, v4, needs to be supported by the
driver.

To select between the different API versions, a new compatible string
format is being introduced: brcm,dpfe-cpu-v<N> where <N> represents the
API version number.

This approach is needed, because the API version has changed in the past
for some chip, which means it cannot be derived reliably just based on
the chip name.

We rely on the boot firmware to populate Device Tree with the proper
compatible string for the correct DPFE API version. The boot firmware
already has this information and can easily pass it to the kernel in
this manner.

Changes since v1:
  - remove compatible string brcm,dpfe-cpu
  - add compatible strings brcm,bcm7278-dpfe-cpu, brcm,bcm7211-dpfe-cpu
  - add "description" section to the binding
  - drop patch that contained API probing and detection code
  - tweak and clarify a few code comments

Markus Mayer (5):
  dt-bindings: memory: remove generic compatible string brcm,dpfe-cpu
  dt-bindings: memory: add two chip-specific compatible string
  dt-bindings: memory: additional compatible strings for Broadcom DPFE
  memory: brcmstb_dpfe: introduce version-specific compatible strings
  memory: brcmstb_dpfe: support DPFE API v4

 .../memory-controllers/brcm,dpfe-cpu.yaml     | 24 ++++++++++-
 drivers/memory/brcmstb_dpfe.c                 | 41 ++++++++++++++++++-
 2 files changed, 62 insertions(+), 3 deletions(-)

-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/5] dt-bindings: memory: remove generic compatible string brcm,dpfe-cpu

[Markus Mayer]

The generic compatible string "brcm,dpfe-cpu" is removed from the
binding as it does not provide any actual benefit.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 .../devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml  | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/5] dt-bindings: memory: remove generic compatible string brcm,dpfe-cpu

[Krzysztof Kozlowski]

On 19/01/2024 22:52, Markus Mayer wrote:
> The generic compatible string "brcm,dpfe-cpu" is removed from the
> binding as it does not provide any actual benefit.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  .../devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml  | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> index 08cbdcddfead..e2b990e4a792 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> @@ -16,7 +16,6 @@ properties:
>        - enum:
>            - brcm,bcm7271-dpfe-cpu
>            - brcm,bcm7268-dpfe-cpu
> -      - const: brcm,dpfe-cpu

We cannot have undocumented compatibles, so I think you wanted to
deprecate it instead. Also, please extend the reasoning from "any actual
benefit". Were there any users? Don't they need it?

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/5] dt-bindings: memory: remove generic compatible string brcm,dpfe-cpu

[Markus Mayer]

On Tue, 23 Jan 2024 at 13:22, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 19/01/2024 22:52, Markus Mayer wrote:
> > The generic compatible string "brcm,dpfe-cpu" is removed from the
> > binding as it does not provide any actual benefit.
> >
> > Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> > ---
> >  .../devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml  | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> > index 08cbdcddfead..e2b990e4a792 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> > @@ -16,7 +16,6 @@ properties:
> >        - enum:
> >            - brcm,bcm7271-dpfe-cpu
> >            - brcm,bcm7268-dpfe-cpu
> > -      - const: brcm,dpfe-cpu
>
> We cannot have undocumented compatibles, so I think you wanted to
> deprecate it instead. Also, please extend the reasoning from "any actual
> benefit". Were there any users? Don't they need it?

Absolutely. I'll change it to deprecate the compatible string instead.

As for the reason deprecating it, it was intended as the "standard"
compatible string that everybody would end up using with the
chip-specific one as fallback should a particular chip require a tweak
down the road. That's why it was introduced. But then the business
with the incompatible DPFE APIs started happening, and the plan to use
"brcm,dpfe-cpu" as the "normal" compatible string that everybody could
use was no longer workable.

Does that make sense as the more in-depth explanation for deprecating it?

Regards,
-Markus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/5] dt-bindings: memory: add two chip-specific compatible string

[Markus Mayer]

Add brcm,bcm7278-dpfe-cpu and brcm,bcm7211-dpfe-cpu to the list of
acceptable compatible strings.

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

diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
index e2b990e4a792..3f00bc2fd3ec 100644
--- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
@@ -16,6 +16,8 @@ properties:
       - enum:
           - brcm,bcm7271-dpfe-cpu
           - brcm,bcm7268-dpfe-cpu
+          - brcm,bcm7278-dpfe-cpu
+          - brcm,bcm7211-dpfe-cpu
 
   reg:
     items:
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] dt-bindings: memory: add two chip-specific compatible string

[Krzysztof Kozlowski]

On 19/01/2024 22:52, Markus Mayer wrote:
> Add brcm,bcm7278-dpfe-cpu and brcm,bcm7211-dpfe-cpu to the list of
> acceptable compatible strings.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  .../devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml   | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> index e2b990e4a792..3f00bc2fd3ec 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> @@ -16,6 +16,8 @@ properties:
>        - enum:
>            - brcm,bcm7271-dpfe-cpu
>            - brcm,bcm7268-dpfe-cpu
> +          - brcm,bcm7278-dpfe-cpu
> +          - brcm,bcm7211-dpfe-cpu

Let's not make it more random than it already is. Feel free to re-order
the two entries to have alphabetical order and then place new entries in
proper places.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] dt-bindings: memory: add two chip-specific compatible string

[Markus Mayer]

On Tue, 23 Jan 2024 at 13:23, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 19/01/2024 22:52, Markus Mayer wrote:
> > Add brcm,bcm7278-dpfe-cpu and brcm,bcm7211-dpfe-cpu to the list of
> > acceptable compatible strings.
> >
> > Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> > ---
> >  .../devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml   | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> > index e2b990e4a792..3f00bc2fd3ec 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> > @@ -16,6 +16,8 @@ properties:
> >        - enum:
> >            - brcm,bcm7271-dpfe-cpu
> >            - brcm,bcm7268-dpfe-cpu
> > +          - brcm,bcm7278-dpfe-cpu
> > +          - brcm,bcm7211-dpfe-cpu
>
> Let's not make it more random than it already is. Feel free to re-order
> the two entries to have alphabetical order and then place new entries in
> proper places.

Will do.

> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/5] dt-bindings: memory: additional compatible strings for Broadcom DPFE

[Markus Mayer]

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.

The chip-specific strings have been kept for compatibility purposes
(hardware is in the field). For new chips, the properly versioned
compatible string should be used.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 .../memory-controllers/brcm,dpfe-cpu.yaml     | 21 ++++++++++++++++++-
 1 file changed, 20 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 3f00bc2fd3ec..42c8160d95d1 100644
--- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
@@ -10,9 +10,28 @@ maintainers:
   - Krzysztof Kozlowski <krzk@kernel.org>
   - Markus Mayer <mmayer@broadcom.com>
 
+description: |
+
+  The DCPU (DDR PHY Front End CPU) interfaces directly with the DDR PHY
+  chip on Broadcom STB SoCs. An API exists for other agents to retrieve
+  or set certain DDR PHY chip parameters by JEDEC.
+
+  Different, incompatible versions of this API have been created over
+  time. The API has changed for the some chips as development progressed
+  and features were added or changed.
+
+  We rely on the boot firmware (which already knows the API version
+  required) to populate Device Tree with the corresponding compatible
+  string.
+
 properties:
   compatible:
     items:
+      - enum:
+          - brcm,dpfe-cpu-v1
+          - brcm,dpfe-cpu-v2
+          - brcm,dpfe-cpu-v3
+          - brcm,dpfe-cpu-v4
       - enum:
           - brcm,bcm7271-dpfe-cpu
           - brcm,bcm7268-dpfe-cpu
@@ -41,7 +60,7 @@ additionalProperties: false
 examples:
   - |
     dpfe-cpu@f1132000 {
-        compatible = "brcm,bcm7271-dpfe-cpu";
+        compatible = "brcm,dpfe-cpu-v1", "brcm,bcm7271-dpfe-cpu";
         reg = <0xf1132000 0x180>,
               <0xf1134000 0x1000>,
               <0xf1138000 0x4000>;
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] dt-bindings: memory: additional compatible strings for Broadcom DPFE

[Krzysztof Kozlowski]

On 19/01/2024 22:52, 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.
> 
> The chip-specific strings have been kept for compatibility purposes
> (hardware is in the field). For new chips, the properly versioned
> compatible string should be used.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  .../memory-controllers/brcm,dpfe-cpu.yaml     | 21 ++++++++++++++++++-
>  1 file changed, 20 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 3f00bc2fd3ec..42c8160d95d1 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> @@ -10,9 +10,28 @@ maintainers:
>    - Krzysztof Kozlowski <krzk@kernel.org>
>    - Markus Mayer <mmayer@broadcom.com>
>  
> +description: |
> +

Drop blank line.

> +  The DCPU (DDR PHY Front End CPU) interfaces directly with the DDR PHY
> +  chip on Broadcom STB SoCs. An API exists for other agents to retrieve
> +  or set certain DDR PHY chip parameters by JEDEC.
> +
> +  Different, incompatible versions of this API have been created over
> +  time. The API has changed for the some chips as development progressed
> +  and features were added or changed.
> +
> +  We rely on the boot firmware (which already knows the API version
> +  required) to populate Device Tree with the corresponding compatible
> +  string.
> +
>  properties:
>    compatible:
>      items:
> +      - enum:
> +          - brcm,dpfe-cpu-v1
> +          - brcm,dpfe-cpu-v2
> +          - brcm,dpfe-cpu-v3
> +          - brcm,dpfe-cpu-v4

I don't see my comment resolved - except more unusual order of
compatibles - and nothing in commit msg explains my previous concerns.

I think my final comment was pretty clear yet ignored completely. There
was no follow up:
https://lore.kernel.org/all/3fff866f-fbe8-4d23-87f3-275380adf3d4@linaro.org/

so with ignored comments: NAK



Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] dt-bindings: memory: additional compatible strings for Broadcom DPFE

[Markus Mayer]

On Tue, 23 Jan 2024 at 13:27, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 19/01/2024 22:52, 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.
> >
> > The chip-specific strings have been kept for compatibility purposes
> > (hardware is in the field). For new chips, the properly versioned
> > compatible string should be used.
> >
> > Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> > ---
> >  .../memory-controllers/brcm,dpfe-cpu.yaml     | 21 ++++++++++++++++++-
> >  1 file changed, 20 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 3f00bc2fd3ec..42c8160d95d1 100644
> > --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> > +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
> > @@ -10,9 +10,28 @@ maintainers:
> >    - Krzysztof Kozlowski <krzk@kernel.org>
> >    - Markus Mayer <mmayer@broadcom.com>
> >
> > +description: |
> > +
>
> Drop blank line.

Will do.

> > +  The DCPU (DDR PHY Front End CPU) interfaces directly with the DDR PHY
> > +  chip on Broadcom STB SoCs. An API exists for other agents to retrieve
> > +  or set certain DDR PHY chip parameters by JEDEC.
> > +
> > +  Different, incompatible versions of this API have been created over
> > +  time. The API has changed for the some chips as development progressed
> > +  and features were added or changed.
> > +
> > +  We rely on the boot firmware (which already knows the API version
> > +  required) to populate Device Tree with the corresponding compatible
> > +  string.
> > +
> >  properties:
> >    compatible:
> >      items:
> > +      - enum:
> > +          - brcm,dpfe-cpu-v1
> > +          - brcm,dpfe-cpu-v2
> > +          - brcm,dpfe-cpu-v3
> > +          - brcm,dpfe-cpu-v4
>
> I don't see my comment resolved - except more unusual order of
> compatibles - and nothing in commit msg explains my previous concerns.

I am confused. What about ordering them in alphabetically ascending
order is unusual?

Which concerns, specifically, are you referencing? I promise I am not
deliberately ignoring concerns as there would be no point in doing so.
I have nothing to gain from it. I am trying to get code accepted into
the kernel. I am not trying to "win any battles" or "prove that I can
be more stubborn" or anything of that nature. If it is possible to
integrate concerns raised by the maintainer, I will gladly do so. And
if not, I'd like to find a way that works for both sides.

BTW, I once used to be on the Linaro Landing Team for Broadcom. You'll
find some commits from me in the kernel that carry a linaro.org e-mail
address. Most are from over a decade ago. Yikes, time flies!

The reason I am mentioning this is to point out that

* I am not new to this.
* I am respecting the Open Source community and so is the rest of the team.
* We are trying to do the right thing and be "good citizens".
* We upstream whatever we can to the relevant project, not just the kernel.
* We aren't on some kind of power-trip or unwilling to listen to feedback.

I am not saying this because I think any of the above makes me special
or particularly accomplished. However, I do think that some things may
need to be clarified as there does seem to be a certain attitude at
play here, with certain assumptions, that is far from constructive.
Hopefully, this has now been cleared up, and we can move forward with
addressing the outstanding concerns regarding our DPFE compatible
strings.

> I think my final comment was pretty clear yet ignored completely. There
> was no follow up:
> https://lore.kernel.org/all/3fff866f-fbe8-4d23-87f3-275380adf3d4@linaro.org/

Unfortunately, it may not be as clear as you think. I did my best to
incorporate what I thought you meant. Clearly that didn't work out so
well.

So, please clarify in more detail, and maybe showing some examples,
what it is you would like to see. For instance, I have no idea what
"(e.g. bcm7271 + v1 + generic fallback)" is actually supposed to mean.
Is that shorthand for 3 compatible strings (brcm,bcm7271-dpfe-cpu,
brcm,dpfe-cpu-v1, brcm,dpfe-cpu). If so, how is that different from
what we already had?

> so with ignored comments: NAK

Like I said before, it is not clear to me what you are looking for.
I'll gladly address any concerns as much as possible.

Regards,
-Markus

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/5] dt-bindings: memory: additional compatible strings for Broadcom DPFE

[Krzysztof Kozlowski]

On 01/02/2024 23:36, Markus Mayer wrote:
> On Tue, 23 Jan 2024 at 13:27, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 19/01/2024 22:52, 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.
>>>
>>> The chip-specific strings have been kept for compatibility purposes
>>> (hardware is in the field). For new chips, the properly versioned
>>> compatible string should be used.
>>>
>>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>>> ---
>>>  .../memory-controllers/brcm,dpfe-cpu.yaml     | 21 ++++++++++++++++++-
>>>  1 file changed, 20 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 3f00bc2fd3ec..42c8160d95d1 100644
>>> --- a/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>> +++ b/Documentation/devicetree/bindings/memory-controllers/brcm,dpfe-cpu.yaml
>>> @@ -10,9 +10,28 @@ maintainers:
>>>    - Krzysztof Kozlowski <krzk@kernel.org>
>>>    - Markus Mayer <mmayer@broadcom.com>
>>>
>>> +description: |
>>> +
>>
>> Drop blank line.
> 
> Will do.
> 
>>> +  The DCPU (DDR PHY Front End CPU) interfaces directly with the DDR PHY
>>> +  chip on Broadcom STB SoCs. An API exists for other agents to retrieve
>>> +  or set certain DDR PHY chip parameters by JEDEC.
>>> +
>>> +  Different, incompatible versions of this API have been created over
>>> +  time. The API has changed for the some chips as development progressed
>>> +  and features were added or changed.
>>> +
>>> +  We rely on the boot firmware (which already knows the API version
>>> +  required) to populate Device Tree with the corresponding compatible
>>> +  string.
>>> +
>>>  properties:
>>>    compatible:
>>>      items:
>>> +      - enum:
>>> +          - brcm,dpfe-cpu-v1
>>> +          - brcm,dpfe-cpu-v2
>>> +          - brcm,dpfe-cpu-v3
>>> +          - brcm,dpfe-cpu-v4
>>
>> I don't see my comment resolved - except more unusual order of
>> compatibles - and nothing in commit msg explains my previous concerns.
> 
> I am confused. What about ordering them in alphabetically ascending
> order is unusual?

Order of entire list - you just added everything to the beginning of the
list. This does not make sense.

> 
> Which concerns, specifically, are you referencing? I promise I am not

That you claim here that bcm7271 is both v1, v2, v3 and v4. Nothing in
the commit msg explains this.

Nothing from my message here:
https://lore.kernel.org/all/3fff866f-fbe8-4d23-87f3-275380adf3d4@linaro.org/
is resolved or addressed.


> deliberately ignoring concerns as there would be no point in doing so.
> I have nothing to gain from it. I am trying to get code accepted into
> the kernel. I am not trying to "win any battles" or "prove that I can
> be more stubborn" or anything of that nature. If it is possible to
> integrate concerns raised by the maintainer, I will gladly do so. And
> if not, I'd like to find a way that works for both sides.
> 
> BTW, I once used to be on the Linaro Landing Team for Broadcom. You'll
> find some commits from me in the kernel that carry a linaro.org e-mail
> address. Most are from over a decade ago. Yikes, time flies!
> 
> The reason I am mentioning this is to point out that
> 
> * I am not new to this.
> * I am respecting the Open Source community and so is the rest of the team.
> * We are trying to do the right thing and be "good citizens".
> * We upstream whatever we can to the relevant project, not just the kernel.
> * We aren't on some kind of power-trip or unwilling to listen to feedback.

OK, I see you sent the almost same patch with no improvements in the
code and in commit msg, so that was the assumption. I made quite clear
concerns last time and asked several questions.

> 
> I am not saying this because I think any of the above makes me special
> or particularly accomplished. However, I do think that some things may
> need to be clarified as there does seem to be a certain attitude at
> play here, with certain assumptions, that is far from constructive.
> Hopefully, this has now been cleared up, and we can move forward with
> addressing the outstanding concerns regarding our DPFE compatible
> strings.
> 
>> I think my final comment was pretty clear yet ignored completely. There
>> was no follow up:
>> https://lore.kernel.org/all/3fff866f-fbe8-4d23-87f3-275380adf3d4@linaro.org/
> 
> Unfortunately, it may not be as clear as you think. I did my best to
> incorporate what I thought you meant. Clearly that didn't work out so
> well.
> 
> So, please clarify in more detail, and maybe showing some examples,
> what it is you would like to see. For instance, I have no idea what
> "(e.g. bcm7271 + v1 + generic fallback)" is actually supposed to mean.

This means: List of three compatibles, SoC specific, your versioned one,
generic compatible used as fallback.

> Is that shorthand for 3 compatible strings (brcm,bcm7271-dpfe-cpu,
> brcm,dpfe-cpu-v1, brcm,dpfe-cpu). If so, how is that different from
> what we already had?

If something is unclear in that message, please continue that message.
There was no further explanation, no further comments on my email, so I
assume you agree or understand it.

> 
>> so with ignored comments: NAK

Also - missing device prefix in subject:

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.


Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/5] memory: brcmstb_dpfe: introduce version-specific compatible strings

[Markus Mayer]

Introduce compatible strings brcm,dpfe-cpu-v1 through brcm,dpfe-cpu-v3
to the Broadcom DPFE driver.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/memory/brcmstb_dpfe.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
index a7ab3d377206..a44a70d028f6 100644
--- a/drivers/memory/brcmstb_dpfe.c
+++ b/drivers/memory/brcmstb_dpfe.c
@@ -924,7 +924,20 @@ static const struct of_device_id brcmstb_dpfe_of_match[] = {
 	{ .compatible = "brcm,bcm7271-dpfe-cpu", .data = &dpfe_api_old_v2 },
 	{ .compatible = "brcm,bcm7278-dpfe-cpu", .data = &dpfe_api_old_v2 },
 	{ .compatible = "brcm,bcm7211-dpfe-cpu", .data = &dpfe_api_new_v2 },
-	/* API v3 is the default going forward */
+
+	/*
+	 * Match the DPFE API flavour based on a versioned compatible string
+	 * that tells us which API version the hardware speaks.
+	 */
+	{ .compatible = "brcm,dpfe-cpu-v1", .data = &dpfe_api_old_v2 },
+	{ .compatible = "brcm,dpfe-cpu-v2", .data = &dpfe_api_new_v2 },
+	{ .compatible = "brcm,dpfe-cpu-v3", .data = &dpfe_api_v3 },
+
+	/*
+	 * This "default" compatible string should no longer be used. It has
+	 * been removed from the binding but kept here for backward
+	 * compatibility.
+	 */
 	{ .compatible = "brcm,dpfe-cpu", .data = &dpfe_api_v3 },
 	{}
 };
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/5] memory: brcmstb_dpfe: introduce version-specific compatible strings

[Krzysztof Kozlowski]

On 19/01/2024 22:52, Markus Mayer wrote:
> Introduce compatible strings brcm,dpfe-cpu-v1 through brcm,dpfe-cpu-v3
> to the Broadcom DPFE driver.

Nothing improved here. I think my last comment was pretty clear what I
expect.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/5] memory: brcmstb_dpfe: introduce version-specific compatible strings

[Markus Mayer]

On Tue, 23 Jan 2024 at 13:28, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 19/01/2024 22:52, Markus Mayer wrote:
> > Introduce compatible strings brcm,dpfe-cpu-v1 through brcm,dpfe-cpu-v3
> > to the Broadcom DPFE driver.
>
> Nothing improved here. I think my last comment was pretty clear what I
> expect.

You are correct. Nothing changed here. I did not get the impression
that you were asking for changes to the actual driver code.

As I said in my response to the previous patch, I am trying my best to
work with maintainers and other community members providing feedback.

Please explain in further detail what you are looking for. Maybe this
will already happen in conjunction with patch 3/5. Your responses
there may already answer my question here. I guess we'll find out.

Regards,
-Markus

> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/5] memory: brcmstb_dpfe: introduce version-specific compatible strings

[Krzysztof Kozlowski]

On 01/02/2024 23:40, Markus Mayer wrote:
> On Tue, 23 Jan 2024 at 13:28, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 19/01/2024 22:52, Markus Mayer wrote:
>>> Introduce compatible strings brcm,dpfe-cpu-v1 through brcm,dpfe-cpu-v3
>>> to the Broadcom DPFE driver.
>>
>> Nothing improved here. I think my last comment was pretty clear what I
>> expect.
> 
> You are correct. Nothing changed here. I did not get the impression
> that you were asking for changes to the actual driver code.

I think my concern was pretty obvious:
"No, why?"

Your commit msg is pointless. Says nothing. It says what you do, but it
is obvious and redundant. I see what you do from the patch diff.

What your commit msg is supposed to say, is explain why this is needed
and what problem are you solving.

This applies to all your commits, to all projects, downstream or
upstream. Repeating what the diff is doing is trivial and does not help
people to understand why this commit is there and what is the commit's
bigger impact.

So after I asked to provide rationale, you send exactly the same commit
without rationale.

And this is repeating in this entire patchset. Patch #1 only mentions
"no actual benefit", but it is discussible. It provides benefit in my
opinion and nothing in your commit msg gives arguments to support your
clause. Patch #2 does not need more explanations but it also does not
make sense in entire series - you want to drop the specific compatibles!
What's more patch #2 does not make any sense with combination of patch
#3 and it is not explained in the commit msg.

Patch #3 also brings zero explanations why you are doing it. From all
four patches, only one had some sort of explanation - patch #1.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/5] memory: brcmstb_dpfe: support DPFE API v4

[Markus Mayer]

Add support for version 4 of the DPFE API. This new version is largely
identical to version 3. The main difference is that all commands now
take the MHS version number as the first argument. Any other arguments
have been pushed down by one (i.e. what used to be arg0 in v3 is arg1 in
v4).

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/memory/brcmstb_dpfe.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/memory/brcmstb_dpfe.c b/drivers/memory/brcmstb_dpfe.c
index a44a70d028f6..97dcb3762666 100644
--- a/drivers/memory/brcmstb_dpfe.c
+++ b/drivers/memory/brcmstb_dpfe.c
@@ -37,6 +37,9 @@
 
 #define DRVNAME			"brcmstb-dpfe"
 
+/* Generic constants */
+#define MHS_VERSION		0x04000000
+
 /* DCPU register offsets */
 #define REG_DCPU_RESET		0x0
 #define REG_TO_DCPU_MBOX	0x10
@@ -301,6 +304,28 @@ static const struct dpfe_api dpfe_api_v3 = {
 	},
 };
 
+/* API v4 firmware commands */
+static struct dpfe_api dpfe_api_v4 = {
+	.version = 4,
+	.fw_name = NULL, /* We expect the firmware to have been downloaded! */
+	.sysfs_attrs = dpfe_v3_groups, /* Same as v3 */
+	.command = {
+		[DPFE_CMD_GET_INFO] = {
+			[MSG_HEADER] = DPFE_MSG_TYPE_COMMAND,
+			[MSG_COMMAND] = 0x0101,
+			[MSG_ARG_COUNT] = 2,
+			[MSG_ARG0] = MHS_VERSION,
+			[MSG_ARG0 + 1] = 1, /* Now the 2nd argument */
+		},
+		[DPFE_CMD_GET_REFRESH] = {
+			[MSG_HEADER] = DPFE_MSG_TYPE_COMMAND,
+			[MSG_COMMAND] = 0x0202,
+			[MSG_ARG_COUNT] = 1,
+			[MSG_ARG0] = MHS_VERSION,
+		},
+	},
+};
+
 static const char *get_error_text(unsigned int i)
 {
 	static const char * const error_text[] = {
@@ -932,6 +957,7 @@ static const struct of_device_id brcmstb_dpfe_of_match[] = {
 	{ .compatible = "brcm,dpfe-cpu-v1", .data = &dpfe_api_old_v2 },
 	{ .compatible = "brcm,dpfe-cpu-v2", .data = &dpfe_api_new_v2 },
 	{ .compatible = "brcm,dpfe-cpu-v3", .data = &dpfe_api_v3 },
+	{ .compatible = "brcm,dpfe-cpu-v4", .data = &dpfe_api_v4 },
 
 	/*
 	 * This "default" compatible string should no longer be used. It has
-- 
2.43.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel