Re: [Patch v7 3/4] dt-bindings: memory: Update reg/reg-names validation

[Dmitry Osipenko]

On 4/13/22 16:37, Rob Herring wrote:
> On Wed, Apr 13, 2022 at 03:10:11PM +0530, Ashish Mhetre wrote:
>> From tegra186 onwards, memory controller support multiple channels.
>> Reg items are updated with address and size of these channels.
>> Tegra186 has overall 5 memory controller channels. Tegra194 and tegra234
>> have overall 17 memory controller channels each.
>> There is 1 reg item for memory controller stream-id registers.
>> So update the reg maxItems to 18 in tegra186 devicetree documentation.
>> Also update validation for reg-names added for these corresponding reg
>> items.
> 
> Somehow your subject should indicate this is for Tegra.
> 
>>
>> Signed-off-by: Ashish Mhetre <amhetre@xxxxxxxxxx>
>> ---
>>  .../nvidia,tegra186-mc.yaml                   | 80 +++++++++++++++++--
>>  1 file changed, 74 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>> index 13c4c82fd0d3..c7cfa6c2cd81 100644
>> --- a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>> +++ b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
>> @@ -34,8 +34,12 @@ properties:
>>            - nvidia,tegra234-mc
>>  
>>    reg:
>> -    minItems: 1
>> -    maxItems: 3
>> +    minItems: 6
> 
> You just broke current users.
> 
>> +    maxItems: 18
>> +
>> +  reg-names:
>> +    minItems: 6
>> +    maxItems: 18
>>  
>>    interrupts:
>>      items:
>> @@ -142,7 +146,18 @@ allOf:
>>      then:
>>        properties:
>>          reg:
>> -          maxItems: 1
>> +          maxItems: 6
>> +          description: 5 memory controller channels and 1 for stream-id registers
>> +
>> +        reg-names:
>> +          maxItems: 6
>> +          items:
>> +            - const: sid
>> +            - const: broadcast
>> +            - const: ch0
>> +            - const: ch1
>> +            - const: ch2
>> +            - const: ch3
>>  
>>    - if:
>>        properties:
>> @@ -151,7 +166,30 @@ allOf:
>>      then:
>>        properties:
>>          reg:
>> -          minItems: 3
>> +          minItems: 18
>> +          description: 17 memory controller channels and 1 for stream-id registers
>> +
>> +        reg-names:
>> +          minItems: 18
>> +          items:
>> +            - const: sid
>> +            - const: broadcast
>> +            - const: ch0
>> +            - const: ch1
>> +            - const: ch2
>> +            - const: ch3
>> +            - const: ch4
>> +            - const: ch5
>> +            - const: ch6
>> +            - const: ch7
>> +            - const: ch8
>> +            - const: ch9
>> +            - const: ch10
>> +            - const: ch11
>> +            - const: ch12
>> +            - const: ch13
>> +            - const: ch14
>> +            - const: ch15
>>  
>>    - if:
>>        properties:
>> @@ -160,13 +198,37 @@ allOf:
>>      then:
>>        properties:
>>          reg:
>> -          minItems: 3
>> +          minItems: 18
>> +          description: 17 memory controller channels and 1 for stream-id registers
>> +
>> +        reg-names:
>> +          minItems: 18
>> +          items:
>> +            - const: sid
>> +            - const: broadcast
>> +            - const: ch0
>> +            - const: ch1
>> +            - const: ch2
>> +            - const: ch3
>> +            - const: ch4
>> +            - const: ch5
>> +            - const: ch6
>> +            - const: ch7
>> +            - const: ch8
>> +            - const: ch9
>> +            - const: ch10
>> +            - const: ch11
>> +            - const: ch12
>> +            - const: ch13
>> +            - const: ch14
>> +            - const: ch15
>>  
>>  additionalProperties: false
>>  
>>  required:
>>    - compatible
>>    - reg
>> +  - reg-names
> 
> New, added properties cannot be required. That's an ABI break.
> 
>>    - interrupts
>>    - "#address-cells"
>>    - "#size-cells"
>> @@ -182,7 +244,13 @@ examples:
>>  
>>          memory-controller@2c00000 {
>>              compatible = "nvidia,tegra186-mc";
>> -            reg = <0x0 0x02c00000 0x0 0xb0000>;
>> +            reg = <0x0 0x02c00000 0x0 0x10000>,    /* MC-SID */
>> +                  <0x0 0x02c10000 0x0 0x10000>,    /* Broadcast channel */
>> +                  <0x0 0x02c20000 0x0 0x10000>,    /* MC0 */
>> +                  <0x0 0x02c30000 0x0 0x10000>,    /* MC1 */
>> +                  <0x0 0x02c40000 0x0 0x10000>,    /* MC2 */
>> +                  <0x0 0x02c50000 0x0 0x10000>;    /* MC3 */
>> +            reg-names = "sid", "broadcast", "ch0", "ch1", "ch2", "ch3";
>>              interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>;
>>  
>>              #address-cells = <2>;
>> -- 
>> 2.17.1
>>

Oh, wait.. I didn't notice that the new reg ranges are only splitting up
the old ranges. Previously it appeared to me that these are the new ranges.

Ashish, in this case you don't need to change the regs in the DT at all.
Instead, you need to specify the per-channel reg-base offsets in the
driver code.