| Message ID | 20220929175147.19749-1-hal.feng@linux.starfivetech.com |
|---|---|
| State | New |
| Headers | show |
| Series | Basic StarFive JH7110 RISC-V SoC support | expand |
On Thu, 29 Sep 2022 13:43:49 -0500, Rob Herring wrote: > On Fri, Sep 30, 2022 at 01:51:47AM +0800, Hal Feng wrote: > > Add bindings for the reset controller on the JH7110 RISC-V > > SoC by StarFive Technology Ltd. > > > > Signed-off-by: Hal Feng <hal.feng@linux.starfivetech.com> > > --- > > .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++ > > 1 file changed, 54 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml > > > > diff --git a/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml > > new file mode 100644 > > index 000000000000..bb0010c200f9 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml > > @@ -0,0 +1,54 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/reset/starfive,jh7110-reset.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: StarFive JH7110 SoC Reset Controller Device Tree Bindings > > + > > +maintainers: > > + - Emil Renner Berthing <kernel@esmil.dk> > > + - Hal Feng <hal.feng@linux.starfivetech.com> > > + > > +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>; 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) { struct starfive_reset *data; int ret; data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); if (!data) return -ENOMEM; data->regmap = device_node_to_regmap(pdev->dev.of_node); //for JH7100 if (IS_ERR(data->regmap)) { data->regmap = syscon_node_to_regmap(pdev->dev.of_node->parent); //for JH7110 if (IS_ERR(data->regmap)) { dev_err(&pdev->dev, "failed to get regmap (error %ld)\n", PTR_ERR(data->regmap)); return PTR_ERR(data->regmap); } } ... } We use this method to avoid errors when remapping the same address in two different drivers, because clock and reset of StarFive JH7110 share a common register address region. For similar implementation, refer to file [1] and [2]. [1] arch/riscv/boot/dts/canaan/k210.dtsi sysctl: syscon@50440000 { compatible = "canaan,k210-sysctl", "syscon", "simple-mfd"; reg = <0x50440000 0x100>; clocks = <&sysclk K210_CLK_APB1>; clock-names = "pclk"; sysclk: clock-controller { #clock-cells = <1>; compatible = "canaan,k210-clk"; clocks = <&in0>; }; sysrst: reset-controller { compatible = "canaan,k210-rst"; #reset-cells = <1>; }; reboot: syscon-reboot { compatible = "syscon-reboot"; regmap = <&sysctl>; offset = <48>; mask = <1>; value = <1>; }; }; [2] drivers/reset/reset-k210.c > > > + > > + "#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? These two properties are the key differences among different reset controllers. 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. After storing them in dt, the reset driver can register all reset controllers with the same compatible string. All we expect is that all reset controllers in a single SoC use the same compatible string for matching and the reset driver can be applied to all StarFive SoCs using different compatible strings. Just like arch/riscv/boot/dts/starfive/jh7100.dtsi: rstgen: reset-controller@11840000 { compatible = "starfive,jh7100-reset"; reg = <0x0 0x11840000 0x0 0x10000>; #reset-cells = <1>; starfive,assert-offset = <0x0>; starfive,status-offset= <0x10>; starfive,nr-resets = <JH7100_RSTN_END>; }; arch/riscv/boot/dts/starfive/jh7110.dtsi: syscrg: syscrg@13020000 { compatible = "syscon", "simple-mfd"; reg = <0x0 0x13020000 0x0 0x10000>; syscrg_clk: clock-controller@13020000 { compatible = "starfive,jh7110-clkgen-sys"; ... }; syscrg_rst: reset-controller@13020000 { compatible = "starfive,jh7110-reset"; #reset-cells = <1>; starfive,assert-offset = <0x2F8>; starfive,status-offset= <0x308>; starfive,nr-resets = <JH7110_SYSRST_END>; }; }; aoncrg: aoncrg@17000000 { compatible = "syscon", "simple-mfd"; reg = <0x0 0x17000000 0x0 0x10000>; aoncrg_clk: clock-controller@17000000 { compatible = "starfive,jh7110-clkgen-aon"; ... }; aoncrg_rst: reset-controller@17000000 { compatible = "starfive,jh7110-reset"; #reset-cells = <1>; starfive,assert-offset = <0x38>; starfive,status-offset= <0x3C>; starfive,nr-resets = <JH7110_AONRST_END>; }; }; stgcrg: stgcrg@10230000 { //Not submmited yet compatible = "syscon", "simple-mfd"; reg = <0x0 0x10230000 0x0 0x10000>; stgcrg_clk: clock-controller@10230000 { compatible = "starfive,jh7110-clkgen-stg"; ... }; stgcrg_rst: reset-controller@10230000 { compatible = "starfive,jh7110-reset"; #reset-cells = <1>; starfive,assert-offset = <0x74>; starfive,status-offset= <0x78>; starfive,nr-resets = <JH7110_STGRST_END>; }; }; ... > > > + > > + starfive,nr-resets: > > + description: Number of reset signals > > + $ref: /schemas/types.yaml#/definitions/uint32 > > Why do you need this? Most bindings don't. If just to validate 'resets' > args, then don't. Can be removed. Instead, the reset driver should includes some related binding headers or defines some macros for pointing out the number of reset signals of each reset controller. Best regards, Hal > > > > + > > +required: > > + - compatible > > + - "#reset-cells" > > + - starfive,assert-offset > > + - starfive,status-offset > > + - starfive,nr-resets > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/reset/starfive-jh7110.h> > > + > > + syscrg_rst: reset-controller@13020000 { > > + compatible = "starfive,jh7110-reset"; > > + #reset-cells = <1>; > > + starfive,assert-offset = <0x2F8>; > > + starfive,status-offset= <0x308>; > > + starfive,nr-resets = <JH7110_SYSRST_END>; > > + }; > > + > > +... > > -- > > 2.17.1 > > > > >
On 11/10/2022 11:30, Hal Feng wrote: > On Thu, 29 Sep 2022 13:43:49 -0500, Rob Herring wrote: >> On Fri, Sep 30, 2022 at 01:51:47AM +0800, Hal Feng wrote: >>> Add bindings for the reset controller on the JH7110 RISC-V >>> SoC by StarFive Technology Ltd. >>> >>> Signed-off-by: Hal Feng <hal.feng@linux.starfivetech.com> >>> --- >>> .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++ >>> 1 file changed, 54 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml >>> new file mode 100644 >>> index 000000000000..bb0010c200f9 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml >>> @@ -0,0 +1,54 @@ >>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/reset/starfive,jh7110-reset.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: StarFive JH7110 SoC Reset Controller Device Tree Bindings >>> + >>> +maintainers: >>> + - Emil Renner Berthing <kernel@esmil.dk> >>> + - Hal Feng <hal.feng@linux.starfivetech.com> >>> + >>> +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. > 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? > > 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. > After storing them in dt, the reset driver can register all reset > controllers with the same compatible string. Which is not how the compatible should be used... > All we expect is that all reset > controllers in a single SoC use the same compatible string for matching and the > reset driver can be applied to all StarFive SoCs using different compatible strings. Keep driver out of the talks. > Just like Existing bad pattern is not an argument to keep it going. Fix bad patterns instead. > > arch/riscv/boot/dts/starfive/jh7100.dtsi: > > rstgen: reset-controller@11840000 { > compatible = "starfive,jh7100-reset"; > reg = <0x0 0x11840000 0x0 0x10000>; > #reset-cells = <1>; > starfive,assert-offset = <0x0>; > starfive,status-offset= <0x10>; > starfive,nr-resets = <JH7100_RSTN_END>; > }; > > arch/riscv/boot/dts/starfive/jh7110.dtsi: > > syscrg: syscrg@13020000 { > compatible = "syscon", "simple-mfd"; > reg = <0x0 0x13020000 0x0 0x10000>; > > syscrg_clk: clock-controller@13020000 { > compatible = "starfive,jh7110-clkgen-sys"; > ... > }; > > syscrg_rst: reset-controller@13020000 { > compatible = "starfive,jh7110-reset"; > #reset-cells = <1>; > starfive,assert-offset = <0x2F8>; > starfive,status-offset= <0x308>; > starfive,nr-resets = <JH7110_SYSRST_END>; > }; > }; > > aoncrg: aoncrg@17000000 { > compatible = "syscon", "simple-mfd"; > reg = <0x0 0x17000000 0x0 0x10000>; > > aoncrg_clk: clock-controller@17000000 { > compatible = "starfive,jh7110-clkgen-aon"; > ... > }; > > aoncrg_rst: reset-controller@17000000 { > compatible = "starfive,jh7110-reset"; > #reset-cells = <1>; > starfive,assert-offset = <0x38>; > starfive,status-offset= <0x3C>; > starfive,nr-resets = <JH7110_AONRST_END>; > }; > }; > > stgcrg: stgcrg@10230000 { //Not submmited yet > compatible = "syscon", "simple-mfd"; > reg = <0x0 0x10230000 0x0 0x10000>; > > stgcrg_clk: clock-controller@10230000 { > compatible = "starfive,jh7110-clkgen-stg"; > ... > }; > > stgcrg_rst: reset-controller@10230000 { > compatible = "starfive,jh7110-reset"; > #reset-cells = <1>; > starfive,assert-offset = <0x74>; > starfive,status-offset= <0x78>; > starfive,nr-resets = <JH7110_STGRST_END>; > }; > }; > ... > >> >>> + >>> + starfive,nr-resets: >>> + description: Number of reset signals >>> + $ref: /schemas/types.yaml#/definitions/uint32 >> >> Why do you need this? Most bindings don't. If just to validate 'resets' >> args, then don't. > > Can be removed. Instead, the reset driver should includes some related > binding headers or defines some macros for pointing out the number of > reset signals of each reset controller. > > Best regards, > Hal > >> >> >>> + >>> +required: >>> + - compatible >>> + - "#reset-cells" >>> + - starfive,assert-offset >>> + - starfive,status-offset >>> + - starfive,nr-resets >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + #include <dt-bindings/reset/starfive-jh7110.h> >>> + >>> + syscrg_rst: reset-controller@13020000 { Please test your patches. Best regards, Krzysztof
On Tue, 11 Oct 2022 at 18:21, Hal Feng <hal.feng@linux.starfivetech.com> wrote: > > On Thu, 29 Sep 2022 13:43:49 -0500, Rob Herring wrote: > > On Fri, Sep 30, 2022 at 01:51:47AM +0800, Hal Feng wrote: > > > Add bindings for the reset controller on the JH7110 RISC-V > > > SoC by StarFive Technology Ltd. > > > > > > Signed-off-by: Hal Feng <hal.feng@linux.starfivetech.com> > > > --- > > > .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++ > > > 1 file changed, 54 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml > > > new file mode 100644 > > > index 000000000000..bb0010c200f9 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml > > > @@ -0,0 +1,54 @@ > > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/reset/starfive,jh7110-reset.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: StarFive JH7110 SoC Reset Controller Device Tree Bindings > > > + > > > +maintainers: > > > + - Emil Renner Berthing <kernel@esmil.dk> > > > + - Hal Feng <hal.feng@linux.starfivetech.com> > > > + > > > +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>; > 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) > { > struct starfive_reset *data; > int ret; > > data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > if (!data) > return -ENOMEM; > > data->regmap = device_node_to_regmap(pdev->dev.of_node); //for JH7100 > if (IS_ERR(data->regmap)) { > data->regmap = syscon_node_to_regmap(pdev->dev.of_node->parent); //for JH7110 > if (IS_ERR(data->regmap)) { > dev_err(&pdev->dev, "failed to get regmap (error %ld)\n", > PTR_ERR(data->regmap)); > return PTR_ERR(data->regmap); > } > } > ... > } > > We use this method to avoid errors when remapping the same address in two > different drivers, because clock and reset of StarFive JH7110 share a common > register address region. For similar implementation, refer to file [1] and [2]. > > [1] arch/riscv/boot/dts/canaan/k210.dtsi > > sysctl: syscon@50440000 { > compatible = "canaan,k210-sysctl", > "syscon", "simple-mfd"; > reg = <0x50440000 0x100>; > clocks = <&sysclk K210_CLK_APB1>; > clock-names = "pclk"; > > sysclk: clock-controller { > #clock-cells = <1>; > compatible = "canaan,k210-clk"; > clocks = <&in0>; > }; > > sysrst: reset-controller { > compatible = "canaan,k210-rst"; > #reset-cells = <1>; > }; > > reboot: syscon-reboot { > compatible = "syscon-reboot"; > regmap = <&sysctl>; > offset = <48>; > mask = <1>; > value = <1>; > }; > }; > > [2] drivers/reset/reset-k210.c Here the syscon makes a little more sense since the same memory area does at least 3 different things, but on the JH7110 it is a dedicated "clock and reset generator", CRG. So this is much better modelled with a single driver taking care of both the clock and resets like the original driver did. If you do git grep reset_controller_register drivers/clk ..you can see that there are lots of other drivers for such peripherals that combine clock and reset. > > > > > + > > > + "#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? > > These two properties are the key differences among different reset controllers. > 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. After storing them in dt, the reset driver can register all reset > controllers with the same compatible string. All we expect is that all reset > controllers in a single SoC use the same compatible string for matching and the > reset driver can be applied to all StarFive SoCs using different compatible strings. > Just like > > arch/riscv/boot/dts/starfive/jh7100.dtsi: > > rstgen: reset-controller@11840000 { > compatible = "starfive,jh7100-reset"; > reg = <0x0 0x11840000 0x0 0x10000>; > #reset-cells = <1>; > starfive,assert-offset = <0x0>; > starfive,status-offset= <0x10>; > starfive,nr-resets = <JH7100_RSTN_END>; > }; > > arch/riscv/boot/dts/starfive/jh7110.dtsi: > > syscrg: syscrg@13020000 { > compatible = "syscon", "simple-mfd"; > reg = <0x0 0x13020000 0x0 0x10000>; > > syscrg_clk: clock-controller@13020000 { > compatible = "starfive,jh7110-clkgen-sys"; > ... > }; > > syscrg_rst: reset-controller@13020000 { > compatible = "starfive,jh7110-reset"; > #reset-cells = <1>; > starfive,assert-offset = <0x2F8>; > starfive,status-offset= <0x308>; > starfive,nr-resets = <JH7110_SYSRST_END>; > }; > }; > > aoncrg: aoncrg@17000000 { > compatible = "syscon", "simple-mfd"; > reg = <0x0 0x17000000 0x0 0x10000>; > > aoncrg_clk: clock-controller@17000000 { > compatible = "starfive,jh7110-clkgen-aon"; > ... > }; > > aoncrg_rst: reset-controller@17000000 { > compatible = "starfive,jh7110-reset"; > #reset-cells = <1>; > starfive,assert-offset = <0x38>; > starfive,status-offset= <0x3C>; > starfive,nr-resets = <JH7110_AONRST_END>; > }; > }; > > stgcrg: stgcrg@10230000 { //Not submmited yet > compatible = "syscon", "simple-mfd"; > reg = <0x0 0x10230000 0x0 0x10000>; > > stgcrg_clk: clock-controller@10230000 { > compatible = "starfive,jh7110-clkgen-stg"; > ... > }; > > stgcrg_rst: reset-controller@10230000 { > compatible = "starfive,jh7110-reset"; > #reset-cells = <1>; > starfive,assert-offset = <0x74>; > starfive,status-offset= <0x78>; > starfive,nr-resets = <JH7110_STGRST_END>; > }; > }; > ... > > > > > > + > > > + starfive,nr-resets: > > > + description: Number of reset signals > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > Why do you need this? Most bindings don't. If just to validate 'resets' > > args, then don't. > > Can be removed. Instead, the reset driver should includes some related > binding headers or defines some macros for pointing out the number of > reset signals of each reset controller. > > Best regards, > Hal > > > > > > > > + > > > +required: > > > + - compatible > > > + - "#reset-cells" > > > + - starfive,assert-offset > > > + - starfive,status-offset > > > + - starfive,nr-resets > > > + > > > +additionalProperties: false > > > + > > > +examples: > > > + - | > > > + #include <dt-bindings/reset/starfive-jh7110.h> > > > + > > > + syscrg_rst: reset-controller@13020000 { > > > + compatible = "starfive,jh7110-reset"; > > > + #reset-cells = <1>; > > > + starfive,assert-offset = <0x2F8>; > > > + starfive,status-offset= <0x308>; > > > + starfive,nr-resets = <JH7110_SYSRST_END>; > > > + }; > > > + > > > +... > > > -- > > > 2.17.1 > > > > > > > > > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On Tue, 11 Oct 2022 12:36:08 -0400, Krzysztof Kozlowski wrote: > On 11/10/2022 11:30, Hal Feng wrote: >> On Thu, 29 Sep 2022 13:43:49 -0500, Rob Herring wrote: >>> On Fri, Sep 30, 2022 at 01:51:47AM +0800, Hal Feng wrote: >>>> Add bindings for the reset controller on the JH7110 RISC-V >>>> SoC by StarFive Technology Ltd. >>>> >>>> Signed-off-by: Hal Feng <hal.feng@linux.starfivetech.com> >>>> --- >>>> .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++ >>>> 1 file changed, 54 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml >>>> >>>> diff --git a/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml >>>> new file mode 100644 >>>> index 000000000000..bb0010c200f9 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml >>>> @@ -0,0 +1,54 @@ >>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >>>> +%YAML 1.2 >>>> +--- >>>> +$id: http://devicetree.org/schemas/reset/starfive,jh7110-reset.yaml# >>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>> + >>>> +title: StarFive JH7110 SoC Reset Controller Device Tree Bindings >>>> + >>>> +maintainers: >>>> + - Emil Renner Berthing <kernel@esmil.dk> >>>> + - Hal Feng <hal.feng@linux.starfivetech.com> >>>> + >>>> +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. 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. 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 >> >> 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. Best regards, Hal > >> After storing them in dt, the reset driver can register all reset >> controllers with the same compatible string. > > Which is not how the compatible should be used... > >> All we expect is that all reset >> controllers in a single SoC use the same compatible string for matching and the >> reset driver can be applied to all StarFive SoCs using different compatible strings. > > Keep driver out of the talks. > >> Just like > > Existing bad pattern is not an argument to keep it going. Fix bad > patterns instead. > >> >> arch/riscv/boot/dts/starfive/jh7100.dtsi: >> >> rstgen: reset-controller@11840000 { >> compatible = "starfive,jh7100-reset"; >> reg = <0x0 0x11840000 0x0 0x10000>; >> #reset-cells = <1>; >> starfive,assert-offset = <0x0>; >> starfive,status-offset= <0x10>; >> starfive,nr-resets = <JH7100_RSTN_END>; >> }; >> >> arch/riscv/boot/dts/starfive/jh7110.dtsi: >> >> syscrg: syscrg@13020000 { >> compatible = "syscon", "simple-mfd"; >> reg = <0x0 0x13020000 0x0 0x10000>; >> >> syscrg_clk: clock-controller@13020000 { >> compatible = "starfive,jh7110-clkgen-sys"; >> ... >> }; >> >> syscrg_rst: reset-controller@13020000 { >> compatible = "starfive,jh7110-reset"; >> #reset-cells = <1>; >> starfive,assert-offset = <0x2F8>; >> starfive,status-offset= <0x308>; >> starfive,nr-resets = <JH7110_SYSRST_END>; >> }; >> }; >> >> aoncrg: aoncrg@17000000 { >> compatible = "syscon", "simple-mfd"; >> reg = <0x0 0x17000000 0x0 0x10000>; >> >> aoncrg_clk: clock-controller@17000000 { >> compatible = "starfive,jh7110-clkgen-aon"; >> ... >> }; >> >> aoncrg_rst: reset-controller@17000000 { >> compatible = "starfive,jh7110-reset"; >> #reset-cells = <1>; >> starfive,assert-offset = <0x38>; >> starfive,status-offset= <0x3C>; >> starfive,nr-resets = <JH7110_AONRST_END>; >> }; >> }; >> >> stgcrg: stgcrg@10230000 { //Not submmited yet >> compatible = "syscon", "simple-mfd"; >> reg = <0x0 0x10230000 0x0 0x10000>; >> >> stgcrg_clk: clock-controller@10230000 { >> compatible = "starfive,jh7110-clkgen-stg"; >> ... >> }; >> >> stgcrg_rst: reset-controller@10230000 { >> compatible = "starfive,jh7110-reset"; >> #reset-cells = <1>; >> starfive,assert-offset = <0x74>; >> starfive,status-offset= <0x78>; >> starfive,nr-resets = <JH7110_STGRST_END>; >> }; >> }; >> ... >> >>> >>>> + >>>> + starfive,nr-resets: >>>> + description: Number of reset signals >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> >>> Why do you need this? Most bindings don't. If just to validate 'resets' >>> args, then don't. >> >> Can be removed. Instead, the reset driver should includes some related >> binding headers or defines some macros for pointing out the number of >> reset signals of each reset controller. >> >> Best regards, >> Hal >> >>> >>> >>>> + >>>> +required: >>>> + - compatible >>>> + - "#reset-cells" >>>> + - starfive,assert-offset >>>> + - starfive,status-offset >>>> + - starfive,nr-resets >>>> + >>>> +additionalProperties: false >>>> + >>>> +examples: >>>> + - | >>>> + #include <dt-bindings/reset/starfive-jh7110.h> >>>> + >>>> + syscrg_rst: reset-controller@13020000 { > > Please test your patches. > > > Best regards, > Krzysztof > >
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
Hey Hal Feng, On Wed, Oct 12, 2022 at 09:33:42AM -0400, Krzysztof Kozlowski wrote: > >>> 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. If two SoCs have exactly the same device/peripheral then they _can_ use the same compatible. If they share some common, viable feature-set then one can "fall back" to the other depending on what your Venn diagram of common features looks like. I've not been following this too closely, but I think what Krzysztof is suggesting is that you have a jh7100 and a jh7110 compatible. Then in your driver you just "know" that if you match against jh7110 which values to use for register offsets & vice versa for a match against the jh7100. There's many examples over the tree for how to handle this sort of thing rather than including it in the devicetree. Maybe Rob and Krzysztof will scream at me for this description, but devicetree is about how periperhals etc are connected together in the system not about the internals of a given peripheral. Following that logic, the devicetree should not contain register offsets etc that are a known quanitity once you've determined that you are running on vendor,soc-foo. Hopefully that helps with your confusion somewhat? Conor.
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@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? 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
On Wed, 12 Oct 2022 15:05:04 +0100, Conor Dooley wrote: > Hey Hal Feng, > > On Wed, Oct 12, 2022 at 09:33:42AM -0400, Krzysztof Kozlowski wrote: > > >>> 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. > > If two SoCs have exactly the same device/peripheral then they _can_ use > the same compatible. If they share some common, viable feature-set then > one can "fall back" to the other depending on what your Venn diagram of > common features looks like. I've not been following this too closely, > but I think what Krzysztof is suggesting is that you have a jh7100 and > a jh7110 compatible. Then in your driver you just "know" that if you > match against jh7110 which values to use for register offsets & vice > versa for a match against the jh7100. There's many examples over the > tree for how to handle this sort of thing rather than including it in > the devicetree. > > Maybe Rob and Krzysztof will scream at me for this description, but > devicetree is about how periperhals etc are connected together in the > system not about the internals of a given peripheral. > > Following that logic, the devicetree should not contain register offsets > etc that are a known quanitity once you've determined that you are running > on vendor,soc-foo. > > Hopefully that helps with your confusion somewhat? > Conor. Yes, anyway, thank you for the detailed reply. Best regards, Hal
diff --git a/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file mode 100644 index 000000000000..bb0010c200f9 --- /dev/null +++ b/Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml @@ -0,0 +1,54 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/reset/starfive,jh7110-reset.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: StarFive JH7110 SoC Reset Controller Device Tree Bindings + +maintainers: + - Emil Renner Berthing <kernel@esmil.dk> + - Hal Feng <hal.feng@linux.starfivetech.com> + +properties: + compatible: + enum: + - starfive,jh7110-reset + + "#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 + + starfive,nr-resets: + description: Number of reset signals + $ref: /schemas/types.yaml#/definitions/uint32 + +required: + - compatible + - "#reset-cells" + - starfive,assert-offset + - starfive,status-offset + - starfive,nr-resets + +additionalProperties: false + +examples: + - | + #include <dt-bindings/reset/starfive-jh7110.h> + + syscrg_rst: reset-controller@13020000 { + compatible = "starfive,jh7110-reset"; + #reset-cells = <1>; + starfive,assert-offset = <0x2F8>; + starfive,status-offset= <0x308>; + starfive,nr-resets = <JH7110_SYSRST_END>; + }; + +...
Add bindings for the reset controller on the JH7110 RISC-V SoC by StarFive Technology Ltd. Signed-off-by: Hal Feng <hal.feng@linux.starfivetech.com> --- .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml