| Message ID | 20220929143225.17907-10-hal.feng@linux.starfivetech.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Basic StarFive JH7110 RISC-V SoC support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 145 lines checked |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Thu, Sep 29, 2022 at 10:32:04PM +0800, Hal Feng wrote: > Store the necessary properties in device tree instead of .c file, > in order to apply this reset driver to other StarFive SoCs. > > Signed-off-by: Hal Feng <hal.feng@linux.starfivetech.com> > --- > .../bindings/reset/starfive,jh7100-reset.yaml | 20 ++++++++ > arch/riscv/boot/dts/starfive/jh7100.dtsi | 3 ++ > drivers/reset/reset-starfive-jh7100.c | 50 +++++++++++++------ > 3 files changed, 57 insertions(+), 16 deletions(-) > > diff --git a/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml b/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml > index 300359a5e14b..3eff3f72a1ed 100644 > --- a/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml > +++ b/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml > @@ -20,19 +20,39 @@ properties: > "#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 > - reg > - "#reset-cells" > + - starfive,assert-offset > + - starfive,status-offset > + - starfive,nr-resets Adding required properties is a red flag. You can't add required properties to an existing binding. That breaks the ABI unless the OS deals with the properties being absent. If the OS has to do that, then why add them in the first place? All this should be implied by the compatible string. Rob
On Fri, 30 Sept 2022 at 22:51, Rob Herring <robh@kernel.org> wrote: > > On Thu, Sep 29, 2022 at 10:32:04PM +0800, Hal Feng wrote: > > Store the necessary properties in device tree instead of .c file, > > in order to apply this reset driver to other StarFive SoCs. > > > > Signed-off-by: Hal Feng <hal.feng@linux.starfivetech.com> > > --- > > .../bindings/reset/starfive,jh7100-reset.yaml | 20 ++++++++ > > arch/riscv/boot/dts/starfive/jh7100.dtsi | 3 ++ > > drivers/reset/reset-starfive-jh7100.c | 50 +++++++++++++------ > > 3 files changed, 57 insertions(+), 16 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml b/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml > > index 300359a5e14b..3eff3f72a1ed 100644 > > --- a/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml > > +++ b/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml > > @@ -20,19 +20,39 @@ properties: > > "#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 > > - reg > > - "#reset-cells" > > + - starfive,assert-offset > > + - starfive,status-offset > > + - starfive,nr-resets > > Adding required properties is a red flag. You can't add required > properties to an existing binding. That breaks the ABI unless the OS > deals with the properties being absent. If the OS has to do that, then > why add them in the first place? All this should be implied by the > compatible string. Indeed. I really don't understand why this is even necessary. As mentioned in my reply to the clock driver my original code just had a combined driver for the whole CRG (clock and reset generator I presume), and then you just need a simple node like this: syscrg: clock-controller@13020000 { compatible = "starfive,jh7110-syscrg"; reg = <0x0 0x13020000 0x0 0x10000>; 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>; #reset-cells = <1>; }; /Emil
diff --git a/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml b/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml index 300359a5e14b..3eff3f72a1ed 100644 --- a/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml +++ b/Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml @@ -20,19 +20,39 @@ properties: "#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 - reg - "#reset-cells" + - starfive,assert-offset + - starfive,status-offset + - starfive,nr-resets additionalProperties: false examples: - | + #include <dt-bindings/reset/starfive-jh7100.h> + reset-controller@11840000 { compatible = "starfive,jh7100-reset"; reg = <0x11840000 0x10000>; #reset-cells = <1>; + starfive,assert-offset = <0x0>; + starfive,status-offset= <0x10>; + starfive,nr-resets = <JH7100_RSTN_END>; }; ... diff --git a/arch/riscv/boot/dts/starfive/jh7100.dtsi b/arch/riscv/boot/dts/starfive/jh7100.dtsi index 000447482aca..904a93411add 100644 --- a/arch/riscv/boot/dts/starfive/jh7100.dtsi +++ b/arch/riscv/boot/dts/starfive/jh7100.dtsi @@ -145,6 +145,9 @@ 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>; }; i2c0: i2c@118b0000 { diff --git a/drivers/reset/reset-starfive-jh7100.c b/drivers/reset/reset-starfive-jh7100.c index 8cba62348a16..d3656e99ae0e 100644 --- a/drivers/reset/reset-starfive-jh7100.c +++ b/drivers/reset/reset-starfive-jh7100.c @@ -14,16 +14,6 @@ #include <dt-bindings/reset/starfive-jh7100.h> -/* register offsets */ -#define JH7100_RESET_ASSERT0 0x00 -#define JH7100_RESET_ASSERT1 0x04 -#define JH7100_RESET_ASSERT2 0x08 -#define JH7100_RESET_ASSERT3 0x0c -#define JH7100_RESET_STATUS0 0x10 -#define JH7100_RESET_STATUS1 0x14 -#define JH7100_RESET_STATUS2 0x18 -#define JH7100_RESET_STATUS3 0x1c - /* * Writing a 1 to the n'th bit of the m'th ASSERT register asserts * line 32m + n, and writing a 0 deasserts the same line. @@ -49,6 +39,10 @@ static const u32 jh7100_reset_asserted[4] = { struct jh7100_reset { struct reset_controller_dev rcdev; struct regmap *regmap; + u32 assert_offset; + u32 status_offset; + u32 nr_resets; + const u32 *asserted; }; static inline struct jh7100_reset * @@ -63,9 +57,9 @@ static int jh7100_reset_update(struct reset_controller_dev *rcdev, struct jh7100_reset *data = jh7100_reset_from(rcdev); u32 offset = id / 32; u32 mask = BIT(id % 32); - u32 reg_assert = JH7100_RESET_ASSERT0 + offset * sizeof(u32); - u32 reg_status = JH7100_RESET_STATUS0 + offset * sizeof(u32); - u32 done = jh7100_reset_asserted[offset] & mask; + u32 reg_assert = data->assert_offset + offset * sizeof(u32); + u32 reg_status = data->status_offset + offset * sizeof(u32); + u32 done = data->asserted ? data->asserted[offset] & mask : 0; u32 value; int ret; @@ -122,7 +116,7 @@ static int jh7100_reset_status(struct reset_controller_dev *rcdev, struct jh7100_reset *data = jh7100_reset_from(rcdev); u32 offset = id / 32; u32 mask = BIT(id % 32); - u32 reg_status = JH7100_RESET_STATUS0 + offset * sizeof(u32); + u32 reg_status = data->status_offset + offset * sizeof(u32); u32 value; int ret; @@ -130,7 +124,7 @@ static int jh7100_reset_status(struct reset_controller_dev *rcdev, if (ret) return ret; - return !((value ^ jh7100_reset_asserted[offset]) & mask); + return !((value ^ data->asserted[offset]) & mask); } static const struct reset_control_ops jh7100_reset_ops = { @@ -143,6 +137,7 @@ static const struct reset_control_ops jh7100_reset_ops = { static int __init jh7100_reset_probe(struct platform_device *pdev) { struct jh7100_reset *data; + int ret; data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); if (!data) @@ -155,12 +150,35 @@ static int __init jh7100_reset_probe(struct platform_device *pdev) return PTR_ERR(data->regmap); } + ret = of_property_read_u32(pdev->dev.of_node, "starfive,assert-offset", + &data->assert_offset); + if (ret < 0) { + dev_err(&pdev->dev, "failed to get starfive,assert-offset: %d\n", ret); + return ret; + } + + ret = of_property_read_u32(pdev->dev.of_node, "starfive,status-offset", + &data->status_offset); + if (ret < 0) { + dev_err(&pdev->dev, "failed to get starfive,status-offset: %d\n", ret); + return ret; + } + + ret = of_property_read_u32(pdev->dev.of_node, "starfive,nr-resets", + &data->nr_resets); + if (ret < 0) { + dev_err(&pdev->dev, "failed to get starfive,nr-resets: %d\n", ret); + return ret; + } + data->rcdev.ops = &jh7100_reset_ops; data->rcdev.owner = THIS_MODULE; - data->rcdev.nr_resets = JH7100_RSTN_END; + data->rcdev.nr_resets = data->nr_resets; data->rcdev.dev = &pdev->dev; data->rcdev.of_node = pdev->dev.of_node; + data->asserted = jh7100_reset_asserted; + return devm_reset_controller_register(&pdev->dev, &data->rcdev); }
Store the necessary properties in device tree instead of .c file, in order to apply this reset driver to other StarFive SoCs. Signed-off-by: Hal Feng <hal.feng@linux.starfivetech.com> --- .../bindings/reset/starfive,jh7100-reset.yaml | 20 ++++++++ arch/riscv/boot/dts/starfive/jh7100.dtsi | 3 ++ drivers/reset/reset-starfive-jh7100.c | 50 +++++++++++++------ 3 files changed, 57 insertions(+), 16 deletions(-)