linux-kernel - Re: [PATCH v1 12/30] dt-bindings: reset: Add starfive,jh7110-reset bindings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <14D22B831DDCFAED+14879d26-5d3a-3487-07ef-3b64e775e43f@linux.starfivetech.com>
Date:   Wed, 12 Oct 2022 22:53:15 +0800
From:   Hal Feng <hal.feng@...ux.starfivetech.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     Rob Herring <robh@...nel.org>, linux-riscv@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-gpio@...r.kernel.org,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Palmer Dabbelt <palmer@...belt.com>,
        Albert Ou <aou@...s.berkeley.edu>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Stephen Boyd <sboyd@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Emil Renner Berthing <kernel@...il.dk>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 12/30] dt-bindings: reset: Add starfive,jh7110-reset
 bindings

On Wed, 12 Oct 2022 09:33:42 -0400, Krzysztof Kozlowski wrote:
> 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@...20000 {
>>>> 		compatible = "syscon", "simple-mfd";
>>>> 		reg = <0x0 0x13020000 0x0 0x10000>;
>>>>
>>>> 		syscrg_clk: clock-controller@...20000 {
>>>> 			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@...20000 {
>>>> 			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?

This node. So not needed.

> 
>> 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.

Okay, I get it. Thanks a lot.

Best regards,
Hal