| Message ID | 20240119215231.758844-4-mmayer@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory: brcmstb_dpfe: support DPFE API v4 | expand |
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
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
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
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>;
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(-)