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: <9f04267d-2592-b303-9b79-9cef672c970a@linaro.org>
Date:   Wed, 12 Oct 2022 09:33:42 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.2
Subject: Re: [PATCH v1 12/30] dt-bindings: reset: Add starfive,jh7110-reset
 bindings
Content-Language: en-US
To:     Hal Feng <hal.feng@linux.starfivetech.com>
Cc:     Rob Herring <robh@kernel.org>, linux-riscv@lists.infradead.org,
        devicetree@vger.kernel.org, linux-clk@vger.kernel.org,
        linux-gpio@vger.kernel.org,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Stephen Boyd <sboyd@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Emil Renner Berthing <kernel@esmil.dk>,
        linux-kernel@vger.kernel.org
References: <20220929143225.17907-1-hal.feng@linux.starfivetech.com>
 <20220929175147.19749-1-hal.feng@linux.starfivetech.com>
 <20220929184349.GA2551443-robh@kernel.org>
 <8BEAFAD2C4CE6E4A+0a00376c-1e3e-f597-bcf6-106ff294859a@linux.starfivetech.com>
 <2f1d1afd-3c97-6ce0-8247-6e1c4a24e548@linaro.org>
 <4769BE3503398017+b1699221-ccc9-a0c1-0b11-141ce9644d74@linux.starfivetech.com>
From:   Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
In-Reply-To: <4769BE3503398017+b1699221-ccc9-a0c1-0b11-141ce9644d74@linux.starfivetech.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/10/2022 09:16, Hal Feng wrote:
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - starfive,jh7110-reset
>>>>
>>>> 'reg' needed? Is this a sub-block of something else?
>>>
>>> Yes, the reset node is a child node of the syscon node, see patch 27 for detail.
>>> You might not see the complete patches at that time due to technical issue of
>>> our smtp email server. Again, I feel so sorry about that.
>>>
>>> 	syscrg: syscrg@13020000 {
>>> 		compatible = "syscon", "simple-mfd";
>>> 		reg = <0x0 0x13020000 0x0 0x10000>;
>>>
>>> 		syscrg_clk: clock-controller@13020000 {
>>> 			compatible = "starfive,jh7110-clkgen-sys";
>>> 			clocks = <&osc>, <&gmac1_rmii_refin>,
>>> 				 <&gmac1_rgmii_rxin>,
>>> 				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
>>> 				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
>>> 				 <&tdm_ext>, <&mclk_ext>;
>>> 			clock-names = "osc", "gmac1_rmii_refin",
>>> 				"gmac1_rgmii_rxin",
>>> 				"i2stx_bclk_ext", "i2stx_lrck_ext",
>>> 				"i2srx_bclk_ext", "i2srx_lrck_ext",
>>> 				"tdm_ext", "mclk_ext";
>>> 			#clock-cells = <1>;
>>> 		};
>>>
>>> 		syscrg_rst: reset-controller@13020000 {
>>> 			compatible = "starfive,jh7110-reset";
>>> 			#reset-cells = <1>;
>>
>> So the answer to the "reg needed?" is what? You have unit address but no
>> reg, so this is not correct.
> 
> Not needed in the reset-controller node, but needed in its parent node. 

We do not talk about parent node. Rob's question was in this bindings.
Is this document a binding for the parent node or for this node?

> I am sorry
> for missing description to point it out in the bindings. I will rewrite all bindings
> for the next version. Unit address here should be deleted.
> 
>>
>>> 			starfive,assert-offset = <0x2F8>;
>>> 			starfive,status-offset= <0x308>;
>>> 			starfive,nr-resets = <JH7110_SYSRST_END>;
>>> 		};
>>> 	};
>>>
>>> In this case, we get the memory mapped space through the parent node with syscon
>>> APIs. You can see patch 13 for detail.
>>>
>>> static int reset_starfive_register(struct platform_device *pdev, const u32 *asserted)
>>> {
>>
>>
>> (...)
>>
>>>
>>>>
>>>>> +
>>>>> +  "#reset-cells":
>>>>> +    const: 1
>>>>> +
>>>>> +  starfive,assert-offset:
>>>>> +    description: Offset of the first ASSERT register
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +
>>>>> +  starfive,status-offset:
>>>>> +    description: Offset of the first STATUS register
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>
>>>> These can't be implied from the compatible string?
> 
> Definitely can. We do this is for simplifying the reset driver.

The role of the bindings is not to simplify some specific driver in some
specific OS...

> Otherwise, we may need to define more compatibles because there
> are multiple reset blocks in JH7110. Another case can be found at
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/reset/altr,rst-mgr.yaml

And why is this a problem? You have different hardware, so should have
different compatibles. Otherwise we would have a compatible
"all,everything" and use it in all possible devices.

>>> These two properties are the key differences among different reset controllers.
>>
>> Different as in different compatibles? Please answer the questions..> 
>>> There are five memory regions for clock and reset in StarFive JH7110 SoC. They
>>> are "syscrg", "aoncrg", "stgcrg", "ispcrg" and "voutcrg". Each memory region
>>> has different reset ASSERT/STATUS register offset and different number of reset
>>> signals. 
>>
>> Then these are not exactly the same devices, so using one compatible for
>> them does not look correct.
> 
> One compatible can just be matched by one device? I think this is what
> confuses me.

I don't understand the question.

> 
> Best regards,
> Hal
> 

Trim the replies - no need to quote everything (entire message following
last reply/quote).

Best regards,
Krzysztof