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

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

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

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

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

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

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