diff mbox series

[v1,09/30] reset: starfive: jh7100: Move necessary properties to device tree

Message ID 20220929143225.17907-10-hal.feng@linux.starfivetech.com
State New
Headers show
Series Basic StarFive JH7110 RISC-V SoC support | expand

Commit Message

Hal Feng Sept. 29, 2022, 2:32 p.m. UTC 
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(-)

Comments

Emil Renner Berthing Oct. 5, 2022, 1:20 p.m. UTC | #1 
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 mbox series

Patch

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);
 }