On Tue, 23 Jan 2024 at 13:27, Krzysztof Kozlowski
<[email protected]> 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 <[email protected]>
> > ---
> > .../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 <[email protected]>
> > - Markus Mayer <[email protected]>
> >
> > +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/[email protected]/
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
On 01/02/2024 23:36, Markus Mayer wrote:
> On Tue, 23 Jan 2024 at 13:27, Krzysztof Kozlowski
> <[email protected]> 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 <[email protected]>
>>> ---
>>> .../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 <[email protected]>
>>> - Markus Mayer <[email protected]>
>>>
>>> +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/[email protected]/
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/[email protected]/
>
> 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