diff mbox series

[16/20] dt-bindings: memory: snps: Detach Zynq DDRC controller support

Message ID 20220822190730.27277-17-Sergey.Semin@baikalelectronics.ru (mailing list archive)
State New, archived
Headers show
Series EDAC/mc/synopsys: Various fixes and cleanups | expand

Commit Message

Serge Semin Aug. 22, 2022, 7:07 p.m. UTC
The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
no any reason to have these controllers described by the same bindings.
Thus let's split them up.

While at it rename the original Synopsys uMCTL2 DT-schema file to a more
descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
description of the device bindings.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 .../snps,dw-umctl2-ddrc.yaml                  | 51 +++++++++++++
 .../memory-controllers/synopsys,ddrc-ecc.yaml | 76 -------------------
 .../xlnx,zynq-ddrc-a05.yaml                   | 38 ++++++++++
 MAINTAINERS                                   |  2 +
 4 files changed, 91 insertions(+), 76 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
 delete mode 100644 Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml

Comments

Krzysztof Kozlowski Aug. 23, 2022, 8:17 a.m. UTC | #1
On 22/08/2022 22:07, Serge Semin wrote:
> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> no any reason to have these controllers described by the same bindings.
> Thus let's split them up.
> 
> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> description of the device bindings.

Filename should be based on compatible, so if renaming then
snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
filename anyway. Therefore nack for rename.

BTW, if you perform renames, generate patches with proper -M/-C/-B
arguments so this is detected.


> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> --->  .../snps,dw-umctl2-ddrc.yaml                  | 51 +++++++++++++

This is a mess. I did not get any cover letters, any other patches any
description of relation between this and your other one.

It seems you make independent and conflicting changes to the same file,
so this has to be properly organized.

Send entire patchset with cover letter with description of all
dependencies to all maintainers.

This is unreviewable now, so a no.

Best regards,
Krzysztof
Krzysztof Kozlowski Aug. 23, 2022, 8:22 a.m. UTC | #2
On 23/08/2022 11:17, Krzysztof Kozlowski wrote:
> On 22/08/2022 22:07, Serge Semin wrote:
>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
>> no any reason to have these controllers described by the same bindings.
>> Thus let's split them up.
>>
>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
>> description of the device bindings.
> 
> Filename should be based on compatible, so if renaming then
> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> filename anyway. Therefore nack for rename.
> 
> BTW, if you perform renames, generate patches with proper -M/-C/-B
> arguments so this is detected.
> 
> 
>>
>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>> --->  .../snps,dw-umctl2-ddrc.yaml                  | 51 +++++++++++++
> 
> This is a mess. I did not get any cover letters, any other patches any
> description of relation between this and your other one.
> 
> It seems you make independent and conflicting changes to the same file,
> so this has to be properly organized.
> 
> Send entire patchset with cover letter with description of all
> dependencies to all maintainers.
> 
> This is unreviewable now, so a no.

And also untestable by Rob's bot, so will have to wait.

I found the cover letters, somehow they got buried in inbox.

Best regards,
Krzysztof
Serge Semin Aug. 23, 2022, 8:25 a.m. UTC | #3
On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
> On 22/08/2022 22:07, Serge Semin wrote:
> > The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> > the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> > uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> > no any reason to have these controllers described by the same bindings.
> > Thus let's split them up.
> > 
> > While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> > descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> > description of the device bindings.
> 
> Filename should be based on compatible, so if renaming then
> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> filename anyway. Therefore nack for rename.
> 
> BTW, if you perform renames, generate patches with proper -M/-C/-B
> arguments so this is detected.
> 
> 
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > --->  .../snps,dw-umctl2-ddrc.yaml                  | 51 +++++++++++++
> 

> This is a mess. I did not get any cover letters, any other patches any
> description of relation between this and your other one.
> 
> It seems you make independent and conflicting changes to the same file,
> so this has to be properly organized.

Don't hurry with the judgement. Have a better look at your DT-related
inbox.
Link: https://lore.kernel.org/linux-devicetree/20220822190730.27277-1-Sergey.Semin@baikalelectronics.ru/

-Sergey

> 
> Send entire patchset with cover letter with description of all
> dependencies to all maintainers.
> 
> This is unreviewable now, so a no.
> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski Aug. 23, 2022, 8:26 a.m. UTC | #4
On 22/08/2022 22:07, Serge Semin wrote:
> -
> -allOf:
> -  - if:
> -      properties:
> -        compatible:
> -          contains:
> -            enum:
> -              - snps,ddrc-3.80a
> -              - xlnx,zynqmp-ddrc-2.40a
> -    then:
> -      required:
> -        - interrupts
> -    else:
> -      properties:
> -        interrupts: false

This is not a pure "rename"... Organize your patches so only one thing
is done at a time. Rename does not change the functional parts of the
binding...

Best regards,
Krzysztof
Serge Semin Aug. 23, 2022, 8:27 a.m. UTC | #5
On Tue, Aug 23, 2022 at 11:22:08AM +0300, Krzysztof Kozlowski wrote:
> On 23/08/2022 11:17, Krzysztof Kozlowski wrote:
> > On 22/08/2022 22:07, Serge Semin wrote:
> >> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> >> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> >> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> >> no any reason to have these controllers described by the same bindings.
> >> Thus let's split them up.
> >>
> >> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> >> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> >> description of the device bindings.
> > 
> > Filename should be based on compatible, so if renaming then
> > snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> > filename anyway. Therefore nack for rename.
> > 
> > BTW, if you perform renames, generate patches with proper -M/-C/-B
> > arguments so this is detected.
> > 
> > 
> >>
> >> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >> --->  .../snps,dw-umctl2-ddrc.yaml                  | 51 +++++++++++++
> > 
> > This is a mess. I did not get any cover letters, any other patches any
> > description of relation between this and your other one.
> > 
> > It seems you make independent and conflicting changes to the same file,
> > so this has to be properly organized.
> > 
> > Send entire patchset with cover letter with description of all
> > dependencies to all maintainers.
> > 
> > This is unreviewable now, so a no.
> 

> And also untestable by Rob's bot, so will have to wait.

For what reason it's untestable? The patch has no dependencies from
any other patchset.

-Sergey

> 
> I found the cover letters, somehow they got buried in inbox.
> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski Aug. 23, 2022, 8:30 a.m. UTC | #6
On 23/08/2022 11:27, Serge Semin wrote:
> On Tue, Aug 23, 2022 at 11:22:08AM +0300, Krzysztof Kozlowski wrote:
>> On 23/08/2022 11:17, Krzysztof Kozlowski wrote:
>>> On 22/08/2022 22:07, Serge Semin wrote:
>>>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
>>>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
>>>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
>>>> no any reason to have these controllers described by the same bindings.
>>>> Thus let's split them up.
>>>>
>>>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
>>>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
>>>> description of the device bindings.
>>>
>>> Filename should be based on compatible, so if renaming then
>>> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
>>> filename anyway. Therefore nack for rename.
>>>
>>> BTW, if you perform renames, generate patches with proper -M/-C/-B
>>> arguments so this is detected.
>>>
>>>
>>>>
>>>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>>>> --->  .../snps,dw-umctl2-ddrc.yaml                  | 51 +++++++++++++
>>>
>>> This is a mess. I did not get any cover letters, any other patches any
>>> description of relation between this and your other one.
>>>
>>> It seems you make independent and conflicting changes to the same file,
>>> so this has to be properly organized.
>>>
>>> Send entire patchset with cover letter with description of all
>>> dependencies to all maintainers.
>>>
>>> This is unreviewable now, so a no.
>>
> 
>> And also untestable by Rob's bot, so will have to wait.
> 
> For what reason it's untestable? The patch has no dependencies from
> any other patchset.

This one is testable, but the next one is not, because it depends on
something. I don't see the reason to split the bindings between
different patchsets.

Best regards,
Krzysztof
Serge Semin Aug. 23, 2022, 8:32 a.m. UTC | #7
On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
> On 22/08/2022 22:07, Serge Semin wrote:
> > The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> > the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> > uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> > no any reason to have these controllers described by the same bindings.
> > Thus let's split them up.
> > 
> > While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> > descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> > description of the device bindings.
> 

> Filename should be based on compatible, so if renaming then
> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> filename anyway. Therefore nack for rename.

New requirement? I've submitted not a single patch to the DT-bindings
sources and didn't get any comment from Rob about that. In addition
There are DT bindings with names different from what is defined in the
compatible name. Moreover there are tons of bindings with various
compatible names. What name to choose then? Finally the current name
is too generic to use for actual DW uMCTL2 DDRC controller.

-Sergey

> 
> BTW, if you perform renames, generate patches with proper -M/-C/-B
> arguments so this is detected.
> 
> 
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > --->  .../snps,dw-umctl2-ddrc.yaml                  | 51 +++++++++++++
> 
> This is a mess. I did not get any cover letters, any other patches any
> description of relation between this and your other one.
> 
> It seems you make independent and conflicting changes to the same file,
> so this has to be properly organized.
> 
> Send entire patchset with cover letter with description of all
> dependencies to all maintainers.
> 
> This is unreviewable now, so a no.
> 
> Best regards,
> Krzysztof
Serge Semin Aug. 23, 2022, 8:36 a.m. UTC | #8
On Tue, Aug 23, 2022 at 11:30:22AM +0300, Krzysztof Kozlowski wrote:
> On 23/08/2022 11:27, Serge Semin wrote:
> > On Tue, Aug 23, 2022 at 11:22:08AM +0300, Krzysztof Kozlowski wrote:
> >> On 23/08/2022 11:17, Krzysztof Kozlowski wrote:
> >>> On 22/08/2022 22:07, Serge Semin wrote:
> >>>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> >>>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> >>>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> >>>> no any reason to have these controllers described by the same bindings.
> >>>> Thus let's split them up.
> >>>>
> >>>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> >>>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> >>>> description of the device bindings.
> >>>
> >>> Filename should be based on compatible, so if renaming then
> >>> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> >>> filename anyway. Therefore nack for rename.
> >>>
> >>> BTW, if you perform renames, generate patches with proper -M/-C/-B
> >>> arguments so this is detected.
> >>>
> >>>
> >>>>
> >>>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> >>>> --->  .../snps,dw-umctl2-ddrc.yaml                  | 51 +++++++++++++
> >>>
> >>> This is a mess. I did not get any cover letters, any other patches any
> >>> description of relation between this and your other one.
> >>>
> >>> It seems you make independent and conflicting changes to the same file,
> >>> so this has to be properly organized.
> >>>
> >>> Send entire patchset with cover letter with description of all
> >>> dependencies to all maintainers.
> >>>
> >>> This is unreviewable now, so a no.
> >>
> > 
> >> And also untestable by Rob's bot, so will have to wait.
> > 
> > For what reason it's untestable? The patch has no dependencies from
> > any other patchset.
> 

> This one is testable, but the next one is not, because it depends on
> something. I don't see the reason to split the bindings between
> different patchsets.

Really, do you want me to collect all 55 patches in a single patchset?

Please read the cover letter more carefully. And please don't hurry
with your judgement before nacking here and there.

-Sergey

> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski Aug. 23, 2022, 8:44 a.m. UTC | #9
On 23/08/2022 11:32, Serge Semin wrote:
> On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
>> On 22/08/2022 22:07, Serge Semin wrote:
>>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
>>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
>>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
>>> no any reason to have these controllers described by the same bindings.
>>> Thus let's split them up.
>>>
>>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
>>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
>>> description of the device bindings.
>>
> 
>> Filename should be based on compatible, so if renaming then
>> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
>> filename anyway. Therefore nack for rename.
> 
> New requirement? I've submitted not a single patch to the DT-bindings
> sources and didn't get any comment from Rob about that. 

This is not a new requirement. It has been since some time and Rob gave
such reviews.

https://lore.kernel.org/linux-devicetree/YlhkwvGdcf4ozTzG@robh.at.kernel.org/

For devices with multiple compatibles that's a bit tricky, but assuming
the bindings describe both original design from Synopsys and it's
implementations, then something closer to Synopsys makes sense.


> In addition
> There are DT bindings with names different from what is defined in the
> compatible name. Moreover there are tons of bindings with various
> compatible names. What name to choose then? Finally the current name
> is too generic to use for actual DW uMCTL2 DDRC controller.

There are thousands of bugs, inconsistencies, naming differences in
kernel. I don't find these as arguments to repeat the practice...so the
bindings file name should be based on the compatible.

Best regards,
Krzysztof
Serge Semin Aug. 23, 2022, 11:45 a.m. UTC | #10
On Tue, Aug 23, 2022 at 11:44:16AM +0300, Krzysztof Kozlowski wrote:
> On 23/08/2022 11:32, Serge Semin wrote:
> > On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
> >> On 22/08/2022 22:07, Serge Semin wrote:
> >>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> >>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> >>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> >>> no any reason to have these controllers described by the same bindings.
> >>> Thus let's split them up.
> >>>
> >>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> >>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> >>> description of the device bindings.
> >>
> > 

> >> Filename should be based on compatible, so if renaming then
> >> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> >> filename anyway. Therefore nack for rename.

Original name was synopsys,ddrc-ecc.yaml which doesn't match any of
the compatible strings. 

> > 
> > New requirement? I've submitted not a single patch to the DT-bindings
> > sources and didn't get any comment from Rob about that. 
> 

> This is not a new requirement. It has been since some time and Rob gave
> such reviews.
> 
> https://lore.kernel.org/linux-devicetree/YlhkwvGdcf4ozTzG@robh.at.kernel.org/

April 2022. So it's new. It would be nice to have it defined somewhere
in docs (writing-bindings.rst?). So does the compatibles order (this
was surprising to me too).

> 
> For devices with multiple compatibles that's a bit tricky, but assuming
> the bindings describe both original design from Synopsys and it's
> implementations, then something closer to Synopsys makes sense.

The closest name would be snps,dw-umctl2-ddrc.yaml. snps,ddrc is too
generic especially for the IP-cores vendor. It doesn't have a
reference to the actual IP-core the device in subject is based on.

> 
> 
> > In addition
> > There are DT bindings with names different from what is defined in the
> > compatible name. Moreover there are tons of bindings with various
> > compatible names. What name to choose then? Finally the current name
> > is too generic to use for actual DW uMCTL2 DDRC controller.
> 

> There are thousands of bugs, inconsistencies, naming differences in
> kernel. I don't find these as arguments to repeat the practice...so the
> bindings file name should be based on the compatible.

Did I ask for an exception? I justified why the renaming was necessary. You
said it goes against the practice of having the DT-schema named as the
device compatible strings and just nacked. But above in this message you said

> "assuming the bindings describe both original design from Synopsys
> and it's implementations, then something closer to Synopsys makes sense"

What I suggest makes more sense than some abstract Synopsys DDRC,
which may refer to a Synopsys DDR controller other than the subject
one. So I see two solutions here:
1. Adding a new generic compatible string like "snps,dw-umctl2-ddrc"
and deprecate the "snps,ddrc-3.80a". It gets to be even more justified
seeing the Synopsys IP-core version has been exported in the device
CSRs since IP-core v3.20a. So having the version attached to the
compatible string was absolutely redundant.
2. Just deprecate the generic compatible string, the new compatible
devices will be supposed to use a vendor-specific compatible strings,
but still rename the DT-bindings file. This makes sense since the
current generic name isn't quiet well structured. It' prefix-part is
too generic and at the same time it refers to a device reversion for
no much reason.

What do you think?

* Note I've got it you'd prefer the renaming being performed in a
separate patch.

-Sergey

> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski Aug. 23, 2022, 12:03 p.m. UTC | #11
On 23/08/2022 14:45, Serge Semin wrote:
> On Tue, Aug 23, 2022 at 11:44:16AM +0300, Krzysztof Kozlowski wrote:
>> On 23/08/2022 11:32, Serge Semin wrote:
>>> On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
>>>> On 22/08/2022 22:07, Serge Semin wrote:
>>>>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
>>>>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
>>>>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
>>>>> no any reason to have these controllers described by the same bindings.
>>>>> Thus let's split them up.
>>>>>
>>>>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
>>>>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
>>>>> description of the device bindings.
>>>>
>>>
> 
>>>> Filename should be based on compatible, so if renaming then
>>>> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
>>>> filename anyway. Therefore nack for rename.
> 
> Original name was synopsys,ddrc-ecc.yaml which doesn't match any of
> the compatible strings. 
> 
>>>
>>> New requirement? I've submitted not a single patch to the DT-bindings
>>> sources and didn't get any comment from Rob about that. 
>>
> 
>> This is not a new requirement. It has been since some time and Rob gave
>> such reviews.
>>
>> https://lore.kernel.org/linux-devicetree/YlhkwvGdcf4ozTzG@robh.at.kernel.org/
> 
> April 2022. So it's new. It would be nice to have it defined somewhere
> in docs (writing-bindings.rst?). So does the compatibles order (this
> was surprising to me too).
> 
>>
>> For devices with multiple compatibles that's a bit tricky, but assuming
>> the bindings describe both original design from Synopsys and it's
>> implementations, then something closer to Synopsys makes sense.
> 
> The closest name would be snps,dw-umctl2-ddrc.yaml. snps,ddrc is too
> generic especially for the IP-cores vendor. It doesn't have a
> reference to the actual IP-core the device in subject is based on.

You are aware that in such case mistake is not in the file name but in
the compatible?

> 
>>
>>
>>> In addition
>>> There are DT bindings with names different from what is defined in the
>>> compatible name. Moreover there are tons of bindings with various
>>> compatible names. What name to choose then? Finally the current name
>>> is too generic to use for actual DW uMCTL2 DDRC controller.
>>
> 
>> There are thousands of bugs, inconsistencies, naming differences in
>> kernel. I don't find these as arguments to repeat the practice...so the
>> bindings file name should be based on the compatible.
> 
> Did I ask for an exception? I justified why the renaming was necessary. You
> said it goes against the practice of having the DT-schema named as the
> device compatible strings and just nacked. But above in this message you said
> 
>> "assuming the bindings describe both original design from Synopsys
>> and it's implementations, then something closer to Synopsys makes sense"
> 
> What I suggest makes more sense than some abstract Synopsys DDRC,
> which may refer to a Synopsys DDR controller other than the subject
> one. So I see two solutions here:
> 1. Adding a new generic compatible string like "snps,dw-umctl2-ddrc"
> and deprecate the "snps,ddrc-3.80a". 

This might be good idea, although unfortunately replacing compatibles
takes quite a lot of time if you do not want to break any out-of-tree
users (read: other users of bindings).


> It gets to be even more justified
> seeing the Synopsys IP-core version has been exported in the device
> CSRs since IP-core v3.20a. So having the version attached to the
> compatible string was absolutely redundant.

The version might have sense in a way to differentiate from some older
versions, pre 3.20a. The binding was probably incomplete anyway.


> 2. Just deprecate the generic compatible string, the new compatible
> devices will be supposed to use a vendor-specific compatible strings,
> but still rename the DT-bindings file. This makes sense since the
> current generic name isn't quiet well structured. It' prefix-part is
> too generic and at the same time it refers to a device reversion for
> no much reason.

You mean disallow entirely "snps,ddrc-3.80a" and expect everyone to use
device/implementation specific compatible? I guess this depends whether
this custom block can be used without vendor specific addons. For
several other DW blocks we expect to have the generic snsp fallback and
this generic fallback can be used alone in Linux (pcie-designware-plat.c
binds to it).

Here the Linux driver also binds to generic synopsys compatible, so I
would assume it has a meaning and use case on its own.

> 
> What do you think?
> 
> * Note I've got it you'd prefer the renaming being performed in a
> separate patch.

The rename could be in the split patch as here, but then I assume the
rename part to be detected by git and be a pure rename. However:
1. The git did not mark it as rename (you might need to use custom
arguments to -M/-B/-C),
2. There were also changes in the process (allOf:if:then).


Best regards,
Krzysztof
Serge Semin Aug. 24, 2022, 5:27 p.m. UTC | #12
On Tue, Aug 23, 2022 at 03:03:51PM +0300, Krzysztof Kozlowski wrote:
> On 23/08/2022 14:45, Serge Semin wrote:
> > On Tue, Aug 23, 2022 at 11:44:16AM +0300, Krzysztof Kozlowski wrote:
> >> On 23/08/2022 11:32, Serge Semin wrote:
> >>> On Tue, Aug 23, 2022 at 11:17:23AM +0300, Krzysztof Kozlowski wrote:
> >>>> On 22/08/2022 22:07, Serge Semin wrote:
> >>>>> The Zynq A05 DDRC controller has nothing in common with DW uMCTL2 DDRC:
> >>>>> the CSRs layout is absolutely different and it doesn't has IRQ unlike DW
> >>>>> uMCTL2 DDR controller of all versions (v1.x, v2.x and v3.x). Thus there is
> >>>>> no any reason to have these controllers described by the same bindings.
> >>>>> Thus let's split them up.
> >>>>>
> >>>>> While at it rename the original Synopsys uMCTL2 DT-schema file to a more
> >>>>> descriptive - snps,dw-umctl2-ddrc.yaml and add a more detailed title and
> >>>>> description of the device bindings.
> >>>>
> >>>
> > 
> >>>> Filename should be based on compatible, so if renaming then
> >>>> snps,ddrc-3.80a.yaml or snps,ddrc.yaml... which leads to original
> >>>> filename anyway. Therefore nack for rename.
> > 
> > Original name was synopsys,ddrc-ecc.yaml which doesn't match any of
> > the compatible strings. 
> > 
> >>>
> >>> New requirement? I've submitted not a single patch to the DT-bindings
> >>> sources and didn't get any comment from Rob about that. 
> >>
> > 
> >> This is not a new requirement. It has been since some time and Rob gave
> >> such reviews.
> >>
> >> https://lore.kernel.org/linux-devicetree/YlhkwvGdcf4ozTzG@robh.at.kernel.org/
> > 
> > April 2022. So it's new. It would be nice to have it defined somewhere
> > in docs (writing-bindings.rst?). So does the compatibles order (this
> > was surprising to me too).
> > 
> >>
> >> For devices with multiple compatibles that's a bit tricky, but assuming
> >> the bindings describe both original design from Synopsys and it's
> >> implementations, then something closer to Synopsys makes sense.
> > 
> > The closest name would be snps,dw-umctl2-ddrc.yaml. snps,ddrc is too
> > generic especially for the IP-cores vendor. It doesn't have a
> > reference to the actual IP-core the device in subject is based on.
> 

> You are aware that in such case mistake is not in the file name but in
> the compatible?

Let's compare, what is defined for Synopsys DW uMCTL2 DDRC in the
DT-bindings now:
compatible string: snps,ddrc-3.80a
file name: synopsys,ddrc-ecc.yaml

First of all they don't match at all. Secondly none of the names refer
to the actual IP-core the DT-bindings file is defined for. So in this
case the problem is in both:
1. file name has undefined vendor-prefix. It's supposed to be "snps".
2. file name has "ecc" suffix, which is a device property, but has
nothing to do with the device actual name.
2. actual device name is different. It's DW uMCTL2 DDRC. Just DDRC
doesn't identify the IP-core in subject.

> 
> > 
> >>
> >>
> >>> In addition
> >>> There are DT bindings with names different from what is defined in the
> >>> compatible name. Moreover there are tons of bindings with various
> >>> compatible names. What name to choose then? Finally the current name
> >>> is too generic to use for actual DW uMCTL2 DDRC controller.
> >>
> > 
> >> There are thousands of bugs, inconsistencies, naming differences in
> >> kernel. I don't find these as arguments to repeat the practice...so the
> >> bindings file name should be based on the compatible.
> > 
> > Did I ask for an exception? I justified why the renaming was necessary. You
> > said it goes against the practice of having the DT-schema named as the
> > device compatible strings and just nacked. But above in this message you said
> > 
> >> "assuming the bindings describe both original design from Synopsys
> >> and it's implementations, then something closer to Synopsys makes sense"
> > 
> > What I suggest makes more sense than some abstract Synopsys DDRC,
> > which may refer to a Synopsys DDR controller other than the subject
> > one. So I see two solutions here:
> > 1. Adding a new generic compatible string like "snps,dw-umctl2-ddrc"
> > and deprecate the "snps,ddrc-3.80a". 
> 

> This might be good idea, although unfortunately replacing compatibles
> takes quite a lot of time if you do not want to break any out-of-tree
> users (read: other users of bindings).

Well, I didn't imply the replacement, but "deprecation". It means to
just mark the "snps,ddrc-3.80a" compatible string as deprecated in the
bindings file and define a new one "snps,dw-umctl2-ddrc" which wouldn't have
the "deprecated: true" property set. It shall at least implicitly warn
people not to add new DTS-files with the deprecated device name. As I
see it eventually the dtbs-check tool will be updated to auto-detect
such attempts and, for instance, print a warning.

> 
> 
> > It gets to be even more justified
> > seeing the Synopsys IP-core version has been exported in the device
> > CSRs since IP-core v3.20a. So having the version attached to the
> > compatible string was absolutely redundant.
> 

> The version might have sense in a way to differentiate from some older
> versions, pre 3.20a.

I've almost fully refactored the device driver, and can say for sure
that the driver doesn't implement any Synopsys IP-core versions
specifics:
1. "xlnx,zynq-ddrc-a05" - has absolutely nothing in common with the
Synopsys DW uMCTL2 DDR controller (see the patch log for details).
That's why I've split the driver and DT-bindings up.
2. "xlnx,zynqmp-ddrc-2.40a" - ZynqMP DDR controller based on the
Synopsys uMCTL2 DDRC v2.40a. The device name is vendor-specific
anyway. So seeing there is no any other ZynqMP DDR controller added,
having IP-core version attached to the compatible string has been
redundant in the first place. Anyway the driver implements only the
ZynqMP-platform specifics irrespective to the IP-core version.
3. "snps,ddrc-3.80a" - without my patchsets this represents a
Synopsys DW uMCTL2 DDR controller synthesized with 64-bit DQ-bus and
8xSDRAM words burst config. The later settings have nothing to do with
the IP-core version, meanwhile what is really v3.x-specific isn't
implemented in the driver.

> The binding was probably incomplete anyway.

At the current state it's incomplete indeed. After applying all my
DT-related patches it will get to be almost complete, at least in the
CSRs, IRQs, clocks and resets resources part. There is always room for
the device-specific properties though, but judging by my experience caught
from Rob' reviews such properties aren't welcome if we have the IP-core
version detectable and if we can add the platform-specific compatible
string. The later one could be used to define the platform-specific
parameters right in the driver so the DT-bindings could be kept
relatively simple.

> 
> 
> > 2. Just deprecate the generic compatible string, the new compatible
> > devices will be supposed to use a vendor-specific compatible strings,
> > but still rename the DT-bindings file. This makes sense since the
> > current generic name isn't quiet well structured. It' prefix-part is
> > too generic and at the same time it refers to a device reversion for
> > no much reason.
> 

> You mean disallow entirely "snps,ddrc-3.80a" and expect everyone to use
> device/implementation specific compatible?

Yes as an alternative to the solution 1. described above.

> I guess this depends whether
> this custom block can be used without vendor specific addons. For
> several other DW blocks we expect to have the generic snsp fallback and
> this generic fallback can be used alone in Linux (pcie-designware-plat.c
> binds to it).

Just recently I've got a very long discussion with Rob regarding the
generic fallback compatible string. What he said "drop the generic
compatible fallback string. It proved to be useless." So I had to drop
the generic fallback compatible string from the nodes of my local DTS
files where it was appropriate and update all my DT-related patches to
disallow having vendor-specific and generic compatible strings specified
together.

Note what Rob said concerned the generic compatible "fallback" case,
not the generic compatible string in general. It's ok to have a
generic device name defined irrespective to the platform vendor.
Moreover it's applicable in case of the DW uMCTL2 DDRC IP-core since
first IP-core version is auto-detectable starting from v3.20a and
second I managed to implement auto-detection solutions for almost
all the DDR/ECC-specific parameters. So I am more inclined to the
solution 1) suggested by me in the previous email message:
- deprecate "snps,ddrc-3.80a" string.
- add new generic "snps,dw-umctl2-ddrc" compatible string.
- rename the DT-bindings file.

> 
> Here the Linux driver also binds to generic synopsys compatible, so I
> would assume it has a meaning and use case on its own.

Please see my messages above regarding the current Synopsys DW uMCTL2
EDAC driver implementation.

> 
> > 
> > What do you think?
> > 
> > * Note I've got it you'd prefer the renaming being performed in a
> > separate patch.
> 
> The rename could be in the split patch as here, but then I assume the
> rename part to be detected by git and be a pure rename. However:
> 1. The git did not mark it as rename (you might need to use custom
> arguments to -M/-B/-C),

Of course git hasn't detected it as rename, because aside with renaming
I've split the bindings up. Splitting these two updates up into two
patches will give us what you said. So to speak I suggest the next
updates for v2:
PATCH X. Detach the Zynq A05 DDRC DT-bindings to a separate schema.
PATCH X + 1. Rename the Synopsys DW uMCTL2 DDRC bindings file and add a more
descriptive generic compatible string name.

What do you think?

> 2. There were also changes in the process (allOf:if:then).

Right. But this is in another patchset. I'll address your notes in there.

-Sergey

> 
> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski Aug. 25, 2022, 6:06 a.m. UTC | #13
On 24/08/2022 20:27, Serge Semin wrote:

> 
> Note what Rob said concerned the generic compatible "fallback" case,
> not the generic compatible string in general. It's ok to have a
> generic device name defined irrespective to the platform vendor.
> Moreover it's applicable in case of the DW uMCTL2 DDRC IP-core since
> first IP-core version is auto-detectable starting from v3.20a and
> second I managed to implement auto-detection solutions for almost
> all the DDR/ECC-specific parameters. So I am more inclined to the
> solution 1) suggested by me in the previous email message:
> - deprecate "snps,ddrc-3.80a" string.
> - add new generic "snps,dw-umctl2-ddrc" compatible string.
> - rename the DT-bindings file.

Sounds ok.

> 
>>
>> Here the Linux driver also binds to generic synopsys compatible, so I
>> would assume it has a meaning and use case on its own.
> 
> Please see my messages above regarding the current Synopsys DW uMCTL2
> EDAC driver implementation.
> 
>>
>>>
>>> What do you think?
>>>
>>> * Note I've got it you'd prefer the renaming being performed in a
>>> separate patch.
>>
>> The rename could be in the split patch as here, but then I assume the
>> rename part to be detected by git and be a pure rename. However:
>> 1. The git did not mark it as rename (you might need to use custom
>> arguments to -M/-B/-C),
> 
> Of course git hasn't detected it as rename, because aside with renaming
> I've split the bindings up. Splitting these two updates up into two
> patches will give us what you said. So to speak I suggest the next
> updates for v2:
> PATCH X. Detach the Zynq A05 DDRC DT-bindings to a separate schema.
> PATCH X + 1. Rename the Synopsys DW uMCTL2 DDRC bindings file and add a more
> descriptive generic compatible string name.
> 
> What do you think?

Regardless of the split the rename can be and should be detected by Git.
That's why we have these options. If it is not detected, you changed too
much during rename, so it is not a rename anymore. Relatively small
amount of changes would still be detected.

> 
>> 2. There were also changes in the process (allOf:if:then).
> 
> Right. But this is in another patchset. I'll address your notes in there.


Best regards,
Krzysztof
Serge Semin Aug. 25, 2022, 1:23 p.m. UTC | #14
On Thu, Aug 25, 2022 at 09:06:42AM +0300, Krzysztof Kozlowski wrote:
> On 24/08/2022 20:27, Serge Semin wrote:
> 
> > 
> > Note what Rob said concerned the generic compatible "fallback" case,
> > not the generic compatible string in general. It's ok to have a
> > generic device name defined irrespective to the platform vendor.
> > Moreover it's applicable in case of the DW uMCTL2 DDRC IP-core since
> > first IP-core version is auto-detectable starting from v3.20a and
> > second I managed to implement auto-detection solutions for almost
> > all the DDR/ECC-specific parameters. So I am more inclined to the
> > solution 1) suggested by me in the previous email message:
> > - deprecate "snps,ddrc-3.80a" string.
> > - add new generic "snps,dw-umctl2-ddrc" compatible string.
> > - rename the DT-bindings file.
> 
> Sounds ok.

Agreed then.

> 
> > 
> >>
> >> Here the Linux driver also binds to generic synopsys compatible, so I
> >> would assume it has a meaning and use case on its own.
> > 
> > Please see my messages above regarding the current Synopsys DW uMCTL2
> > EDAC driver implementation.
> > 
> >>
> >>>
> >>> What do you think?
> >>>
> >>> * Note I've got it you'd prefer the renaming being performed in a
> >>> separate patch.
> >>
> >> The rename could be in the split patch as here, but then I assume the
> >> rename part to be detected by git and be a pure rename. However:
> >> 1. The git did not mark it as rename (you might need to use custom
> >> arguments to -M/-B/-C),
> > 
> > Of course git hasn't detected it as rename, because aside with renaming
> > I've split the bindings up. Splitting these two updates up into two
> > patches will give us what you said. So to speak I suggest the next
> > updates for v2:
> > PATCH X. Detach the Zynq A05 DDRC DT-bindings to a separate schema.
> > PATCH X + 1. Rename the Synopsys DW uMCTL2 DDRC bindings file and add a more
> > descriptive generic compatible string name.
> > 
> > What do you think?
> 

> Regardless of the split the rename can be and should be detected by Git.
> That's why we have these options. If it is not detected, you changed too
> much during rename, so it is not a rename anymore. Relatively small
> amount of changes would still be detected.

Right. I'll make sure the renaming is detected.

-Sergey

> 
> > 
> >> 2. There were also changes in the process (allOf:if:then).
> > 
> > Right. But this is in another patchset. I'll address your notes in there.
> 
> 
> Best regards,
> Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
new file mode 100644
index 000000000000..787d91d64eee
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
@@ -0,0 +1,51 @@ 
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/snps,dw-umctl2-ddrc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys DesignWare Universal Multi-Protocol Memory Controller
+
+maintainers:
+  - Krzysztof Kozlowski <krzk@kernel.org>
+  - Manish Narani <manish.narani@xilinx.com>
+  - Michal Simek <michal.simek@xilinx.com>
+
+description: |
+  Synopsys DesignWare Enhanced uMCTL2 DDR Memory Controller is cappable of
+  working with DDR devices up to (LP)DDR4 protocol. It can be equipped
+  with SEC/DEC ECC feature if DRAM data bus width is either 16-bits or
+  32-bits or 64-bits wide.
+
+  The ZynqMP DDR controller is based on the DW uMCTL2 v2.40a controller.
+  It has an optional SEC/DEC ECC support in 64-bit and 32-bit bus width
+  configurations.
+
+properties:
+  compatible:
+    enum:
+      - snps,ddrc-3.80a
+      - xlnx,zynqmp-ddrc-2.40a
+
+  interrupts:
+    maxItems: 1
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+
+additionalProperties: false
+
+examples:
+  - |
+    memory-controller@fd070000 {
+      compatible = "xlnx,zynqmp-ddrc-2.40a";
+      reg = <0xfd070000 0x30000>;
+      interrupt-parent = <&gic>;
+      interrupts = <0 112 4>;
+    };
+...
diff --git a/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml b/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
deleted file mode 100644
index f46e95704f53..000000000000
--- a/Documentation/devicetree/bindings/memory-controllers/synopsys,ddrc-ecc.yaml
+++ /dev/null
@@ -1,76 +0,0 @@ 
-# SPDX-License-Identifier: GPL-2.0-only
-%YAML 1.2
----
-$id: http://devicetree.org/schemas/memory-controllers/synopsys,ddrc-ecc.yaml#
-$schema: http://devicetree.org/meta-schemas/core.yaml#
-
-title: Synopsys IntelliDDR Multi Protocol memory controller
-
-maintainers:
-  - Krzysztof Kozlowski <krzk@kernel.org>
-  - Manish Narani <manish.narani@xilinx.com>
-  - Michal Simek <michal.simek@xilinx.com>
-
-description: |
-  The ZynqMP DDR ECC controller has an optional ECC support in 64-bit and
-  32-bit bus width configurations.
-
-  The Zynq DDR ECC controller has an optional ECC support in half-bus width
-  (16-bit) configuration.
-
-  These both ECC controllers correct single bit ECC errors and detect double bit
-  ECC errors.
-
-properties:
-  compatible:
-    enum:
-      - snps,ddrc-3.80a
-      - xlnx,zynq-ddrc-a05
-      - xlnx,zynqmp-ddrc-2.40a
-
-  interrupts:
-    maxItems: 1
-
-  reg:
-    maxItems: 1
-
-required:
-  - compatible
-  - reg
-
-allOf:
-  - if:
-      properties:
-        compatible:
-          contains:
-            enum:
-              - snps,ddrc-3.80a
-              - xlnx,zynqmp-ddrc-2.40a
-    then:
-      required:
-        - interrupts
-    else:
-      properties:
-        interrupts: false
-
-additionalProperties: false
-
-examples:
-  - |
-    memory-controller@f8006000 {
-        compatible = "xlnx,zynq-ddrc-a05";
-        reg = <0xf8006000 0x1000>;
-    };
-
-  - |
-    axi {
-        #address-cells = <2>;
-        #size-cells = <2>;
-
-        memory-controller@fd070000 {
-            compatible = "xlnx,zynqmp-ddrc-2.40a";
-            reg = <0x0 0xfd070000 0x0 0x30000>;
-            interrupt-parent = <&gic>;
-            interrupts = <0 112 4>;
-        };
-    };
diff --git a/Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml b/Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml
new file mode 100644
index 000000000000..8f72e2f8588a
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml
@@ -0,0 +1,38 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/memory-controllers/xlnx,zynq-ddrc-a05.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Zynq A05 DDR Memory Controller
+
+maintainers:
+  - Krzysztof Kozlowski <krzk@kernel.org>
+  - Manish Narani <manish.narani@xilinx.com>
+  - Michal Simek <michal.simek@xilinx.com>
+
+description:
+  The Zynq DDR ECC controller has an optional ECC support in half-bus width
+  (16-bit) configuration. It is cappable of correcting single bit ECC errors
+  and detecting double bit ECC errors.
+
+properties:
+  compatible:
+    const: xlnx,zynq-ddrc-a05
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    memory-controller@f8006000 {
+      compatible = "xlnx,zynq-ddrc-a05";
+      reg = <0xf8006000 0x1000>;
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 9d7f64dc0efe..17f23f810ee4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3086,7 +3086,9 @@  S:	Supported
 W:	http://wiki.xilinx.com
 T:	git https://github.com/Xilinx/linux-xlnx.git
 F:	Documentation/devicetree/bindings/i2c/cdns,i2c-r1p10.yaml
+F:	Documentation/devicetree/bindings/memory-controllers/snps,dw-umctl2-ddrc.yaml
 F:	Documentation/devicetree/bindings/i2c/xlnx,xps-iic-2.00.a.yaml
+F:	Documentation/devicetree/bindings/memory-controllers/xlnx,zynq-ddrc-a05.yaml
 F:	Documentation/devicetree/bindings/spi/xlnx,zynq-qspi.yaml
 F:	arch/arm/mach-zynq/
 F:	drivers/clocksource/timer-cadence-ttc.c