Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ee81e955-32be-66ea-377b-263ee60a2632@linaro.org>
Date:   Mon, 17 Jul 2023 22:47:53 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH 1/2] dt-bindings: reset: Updated binding for Versal-NET
 reset driver
Content-Language: en-US
To:     Conor Dooley <conor@kernel.org>,
        Piyush Mehta <piyush.mehta@amd.com>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        conor+dt@kernel.org, p.zabel@pengutronix.de, michal.simek@amd.com,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        git@amd.com
References: <20230717112348.1381367-1-piyush.mehta@amd.com>
 <20230717112348.1381367-2-piyush.mehta@amd.com>
 <20230717-explode-caucus-82c12e340e39@spud>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <20230717-explode-caucus-82c12e340e39@spud>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 17/07/2023 20:40, Conor Dooley wrote:
> On Mon, Jul 17, 2023 at 04:53:47PM +0530, Piyush Mehta wrote:
>> Added documentation and Versal-NET reset indices to describe about
>> Versal-Net reset driver bindings.
>>
>> In Versal-NET all reset indices includes Class, SubClass, Type, Index
>> information whereas class refers to clock, reset, power etc.,
>> Underlying firmware in Versal have such classification and expects
>> the ID to be this way.
>> [13:0] - Index bits
>> [19:14] - Type bits
>> [25:20] - SubClass bits
>> [31:26] - Class bits.
> 
> Riight.. I'm not sure that describing these as "indices" is really all
> that valid, given only 13:0 are actually the index.
> I'd be inclined to say that the type/class/subclass stuff should not be
> part of the dt-bindings, and instead looked up inside the driver
> depending on the index.
> 
> Hopefully Rob or Krzysztof can comment further.

This confuses me as well. I don't understand why do you need it in the
bindings. Nothing uses these values, so storing them as bindings seems
pointless.
> 
> Thanks,
> Conor.
> 
>>
>> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
>> ---
>>  .../bindings/reset/xlnx,zynqmp-reset.yaml     |   4 +
>>  .../reset/xlnx-versal-net-resets.h            | 127 ++++++++++++++++++
>>  2 files changed, 131 insertions(+)
>>  create mode 100644 include/dt-bindings/reset/xlnx-versal-net-resets.h
>>
>> diff --git a/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml b/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
>> index 0d50f6a54af3..b996fc1d4e53 100644
>> --- a/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
>> +++ b/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
>> @@ -27,11 +27,15 @@ description: |
>>    For list of all valid reset indices for Versal
>>    <dt-bindings/reset/xlnx-versal-resets.h>
>>  
>> +  For list of all valid reset indices for Versal-NET
>> +  <dt-bindings/reset/xlnx-versal-net-resets.h>
>> +
>>  properties:
>>    compatible:
>>      enum:
>>        - xlnx,zynqmp-reset
>>        - xlnx,versal-reset
>> +      - xlnx,versal-net-reset
>>  
>>    "#reset-cells":
>>      const: 1
>> diff --git a/include/dt-bindings/reset/xlnx-versal-net-resets.h b/include/dt-bindings/reset/xlnx-versal-net-resets.h
>> new file mode 100644
>> index 000000000000..b3e7d5e4c33e
>> --- /dev/null
>> +++ b/include/dt-bindings/reset/xlnx-versal-net-resets.h
>> @@ -0,0 +1,127 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + *  Copyright (C) 2023 Advanced Micro Devices, Inc. All Rights Reserved.
>> + */
>> +
>> +#ifndef _DT_BINDINGS_VERSAL_NET_RESETS_H
>> +#define _DT_BINDINGS_VERSAL_NET_RESETS_H
>> +
>> +#define PM_RST_PMC_POR                          (0xc30c001U)

IDs start from 0 and are incremented by 1.

Best regards,
Krzysztof