diff mbox series

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

Message ID 20220413094012.13589-4-amhetre@nvidia.com
State Changes Requested
Headers show
Series memory: tegra: Add MC channels and error logging | expand

Commit Message

Ashish Mhetre April 13, 2022, 9:40 a.m. UTC
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.

Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
---
 .../nvidia,tegra186-mc.yaml                   | 80 +++++++++++++++++--
 1 file changed, 74 insertions(+), 6 deletions(-)

Comments

Rob Herring April 13, 2022, 12:26 p.m. UTC | #1
On Wed, 13 Apr 2022 15:10:11 +0530, Ashish Mhetre wrote:
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/


memory-controller@2c00000: reg: [[0, 46137344, 0, 720896]] is too short
	arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra186-p3509-0000+p3636-0001.dtb
	arch/arm64/boot/dts/nvidia/tegra186-p3509-0000+p3636-0001.dtb

memory-controller@2c00000: reg: [[46137344, 1048576], [45613056, 262144], [24117248, 1048576]] is too short
	arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dtb
	arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dtb
	arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra234-sim-vdk.dtb
	arch/arm64/boot/dts/nvidia/tegra234-sim-vdk.dtb

memory-controller@2c00000: 'reg-names' is a required property
	arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra186-p3509-0000+p3636-0001.dtb
	arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra194-p3509-0000+p3668-0001.dtb
	arch/arm64/boot/dts/nvidia/tegra234-p3737-0000+p3701-0000.dtb
	arch/arm64/boot/dts/nvidia/tegra234-sim-vdk.dtb
Rob Herring April 13, 2022, 1:37 p.m. UTC | #2
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@nvidia.com>
> ---
>  .../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
>
Dmitry Osipenko April 13, 2022, 2:04 p.m. UTC | #3
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@nvidia.com>
>> ---
>>  .../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.
Ashish Mhetre April 13, 2022, 4:17 p.m. UTC | #4
On 4/13/2022 7:34 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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@nvidia.com>
>>> ---
>>>   .../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.

Yes, it's kind of splitting up the old ranges and straight forward for
Tegra186. But on Tegra194 and Tegra234 the old address is not in single
range. It's already split across 3 ranges. We have to choose right range
and add channel offsets to that range in order to read interrupts.
So I went with the approach of splitting the regs in DT itself as per
the channels because that way they can be mapped in a single loop and
used easily.
If we want to specify per-channel reg-base offsets then that would be
per-SOC. Also we would need to choose correct reg-range for Tegra194 and
Tegra234 and have a way to maintain offsets of channels from those
respective reg-ranges.
Dmitry Osipenko April 13, 2022, 9:09 p.m. UTC | #5
On 4/13/22 19:17, Ashish Mhetre wrote:
> 
> 
> On 4/13/2022 7:34 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 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@nvidia.com>
>>>> ---
>>>>   .../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.
> 
> Yes, it's kind of splitting up the old ranges and straight forward for
> Tegra186. But on Tegra194 and Tegra234 the old address is not in single
> range. It's already split across 3 ranges. We have to choose right range
> and add channel offsets to that range in order to read interrupts.
> So I went with the approach of splitting the regs in DT itself as per
> the channels because that way they can be mapped in a single loop and
> used easily.
> If we want to specify per-channel reg-base offsets then that would be
> per-SOC. Also we would need to choose correct reg-range for Tegra194 and
> Tegra234 and have a way to maintain offsets of channels from those
> respective reg-ranges.

That is not nice too. Should be better to switch to the new DT scheme,
since those channels weren't used by older kernels. It's okay to change
the binding ABI in this case then, driver will continue to work for the
older dtbs. Have you tested driver using the older dtbs?
Ashish Mhetre April 14, 2022, 4:07 a.m. UTC | #6
On 4/14/2022 2:39 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 4/13/22 19:17, Ashish Mhetre wrote:
>>
>>
>> On 4/13/2022 7:34 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 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@nvidia.com>
>>>>> ---
>>>>>    .../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.
>>
>> Yes, it's kind of splitting up the old ranges and straight forward for
>> Tegra186. But on Tegra194 and Tegra234 the old address is not in single
>> range. It's already split across 3 ranges. We have to choose right range
>> and add channel offsets to that range in order to read interrupts.
>> So I went with the approach of splitting the regs in DT itself as per
>> the channels because that way they can be mapped in a single loop and
>> used easily.
>> If we want to specify per-channel reg-base offsets then that would be
>> per-SOC. Also we would need to choose correct reg-range for Tegra194 and
>> Tegra234 and have a way to maintain offsets of channels from those
>> respective reg-ranges.
> 
> That is not nice too. Should be better to switch to the new DT scheme,
> since those channels weren't used by older kernels. It's okay to change
> the binding ABI in this case then, driver will continue to work for the
> older dtbs.

So the current DTS and binding changes are fine?

> Have you tested driver using the older dtbs?

Yes, the driver is tested with old dtb and it's working fine.
Dmitry Osipenko April 14, 2022, 8:35 p.m. UTC | #7
On 4/14/22 07:07, Ashish Mhetre wrote:
> 
> 
> On 4/14/2022 2:39 AM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 4/13/22 19:17, Ashish Mhetre wrote:
>>>
>>>
>>> On 4/13/2022 7:34 PM, Dmitry Osipenko wrote:
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> 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@nvidia.com>
>>>>>> ---
>>>>>>    .../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.
>>>
>>> Yes, it's kind of splitting up the old ranges and straight forward for
>>> Tegra186. But on Tegra194 and Tegra234 the old address is not in single
>>> range. It's already split across 3 ranges. We have to choose right range
>>> and add channel offsets to that range in order to read interrupts.
>>> So I went with the approach of splitting the regs in DT itself as per
>>> the channels because that way they can be mapped in a single loop and
>>> used easily.
>>> If we want to specify per-channel reg-base offsets then that would be
>>> per-SOC. Also we would need to choose correct reg-range for Tegra194 and
>>> Tegra234 and have a way to maintain offsets of channels from those
>>> respective reg-ranges.
>>
>> That is not nice too. Should be better to switch to the new DT scheme,
>> since those channels weren't used by older kernels. It's okay to change
>> the binding ABI in this case then, driver will continue to work for the
>> older dtbs.
> 
> So the current DTS and binding changes are fine?

It's fine to me. Doesn't hurt to explain in the commit message that the
ABI change is intended and it's compatible with the previous ABI.

>> Have you tested driver using the older dtbs?
> 
> Yes, the driver is tested with old dtb and it's working fine.

Ok
Ashish Mhetre April 17, 2022, 9:27 a.m. UTC | #8
On 4/13/2022 7:07 PM, Rob Herring wrote:
> External email: Use caution opening links or attachments
> 
> 
> 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.
> 
Okay, I'll update the subject.

>>
>> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
>> ---
>>   .../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.
> 
The DTS changes are handled in driver. The driver is compatible with
older DTS as well. dt bindings check will fail but that will request
the users to switch to new DTS. Will that be fine?

>> +    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.
> 
This is handled in driver code to make sure driver works with old dts
as well. So is this bindings change fine or shall I change it such that
dt bindings check shall pass with older dts as well?
Or as mentioned by Dmitry, I can update the commit message to reflect
that ABI change is intended and driver is compatible with older DTBs as
well.

>>     - 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
>>
Ashish Mhetre April 24, 2022, 5:20 a.m. UTC | #9
On 4/17/2022 2:57 PM, Ashish Mhetre wrote:
> 
> 
> On 4/13/2022 7:07 PM, Rob Herring wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 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.
>>
> Okay, I'll update the subject.
> 
>>>
>>> Signed-off-by: Ashish Mhetre <amhetre@nvidia.com>
>>> ---
>>>   .../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.
>>
> The DTS changes are handled in driver. The driver is compatible with
> older DTS as well. dt bindings check will fail but that will request
> the users to switch to new DTS. Will that be fine?
> 
>>> +    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.
>>
> This is handled in driver code to make sure driver works with old dts
> as well. So is this bindings change fine or shall I change it such that
> dt bindings check shall pass with older dts as well?
> Or as mentioned by Dmitry, I can update the commit message to reflect
> that ABI change is intended and driver is compatible with older DTBs as
> well.
> 
Hi Rob,
Can you please confirm how shall I go in next version?
Is it fine for dt bindings check to fail if driver is compatible with
old as well as new dts? Or dt bindings check shall pass with old as
well as new dts?

>>>     - 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
>>>
Krzysztof Kozlowski April 24, 2022, 2:34 p.m. UTC | #10
On 24/04/2022 07:20, Ashish Mhetre wrote:
>>>
>>> New, added properties cannot be required. That's an ABI break.
>>>
>> This is handled in driver code to make sure driver works with old dts
>> as well. So is this bindings change fine or shall I change it such that
>> dt bindings check shall pass with older dts as well?
>> Or as mentioned by Dmitry, I can update the commit message to reflect
>> that ABI change is intended and driver is compatible with older DTBs as
>> well.
>>
> Hi Rob,
> Can you please confirm how shall I go in next version?
> Is it fine for dt bindings check to fail if driver is compatible with
> old as well as new dts? Or dt bindings check shall pass with old as
> well as new dts?
The driver works fine without reg-names and accepts old DTB, right? In
such case, just mention this in commit msg, that the bindings require
reg-names but backwards compatibility will be preserved in the driver. I
think it's fine to alter bindings such way.

Best regards,
Krzysztof
Ashish Mhetre April 25, 2022, 4:48 a.m. UTC | #11
On 4/24/2022 8:04 PM, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
> 
> 
> On 24/04/2022 07:20, Ashish Mhetre wrote:
>>>>
>>>> New, added properties cannot be required. That's an ABI break.
>>>>
>>> This is handled in driver code to make sure driver works with old dts
>>> as well. So is this bindings change fine or shall I change it such that
>>> dt bindings check shall pass with older dts as well?
>>> Or as mentioned by Dmitry, I can update the commit message to reflect
>>> that ABI change is intended and driver is compatible with older DTBs as
>>> well.
>>>
>> Hi Rob,
>> Can you please confirm how shall I go in next version?
>> Is it fine for dt bindings check to fail if driver is compatible with
>> old as well as new dts? Or dt bindings check shall pass with old as
>> well as new dts?
> The driver works fine without reg-names and accepts old DTB, right? In
> such case, just mention this in commit msg, that the bindings require
> reg-names but backwards compatibility will be preserved in the driver. I
> think it's fine to alter bindings such way.
> 
Thanks for confirming Krzysztof. I'll send v8 with this information.

> Best regards,
> Krzysztof
diff mbox series

Patch

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
+    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
   - 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>;