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

[Ashish Mhetre <amhetre@nvidia.com>]

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.