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 (mailing list archive)
State New, archived
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

Rob Herring (Arm) Sept. 30, 2022, 8:49 p.m. UTC | #1
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
Emil Renner Berthing Oct. 5, 2022, 1:20 p.m. UTC | #2
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);
 }