LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/4] clk: ralink: make system controller a reset provider
@ 2021-10-06  6:12 Sergio Paracuellos
  2021-10-06  6:12 ` [PATCH 1/4] dt-bindings: reset: add dt binding header for Mediatek MT7621 resets Sergio Paracuellos
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Sergio Paracuellos @ 2021-10-06  6:12 UTC (permalink / raw)
  To: sboyd
  Cc: linux-clk, gregkh, devicetree, robh+dt, linux-staging, neil,
	linux-kernel, john

Hi all,

This patch series add minimal change to provide mt7621 resets properly
defining them in the 'mediatek,mt7621-sysc' node which is the system
controller of the SoC and is already providing clocks to the rest of
the world.

There is shared architecture code for all ralink platforms in 'reset.c'
file located in 'arch/mips/ralink' but the correct thing to do to align
hardware with software seems to define and add related reset code to the 
already mainlined clock driver.

After this changes, we can get rid of the useless reset controller node
in the device tree and use system controller node instead where the property
'#reset-cells' has been added. Binding documentation for this nodeq has
been updated with the new property accordly.

This series also provide a bindings include header where all related
reset bits for the MT7621 SoC are defined.

Also, please take a look to this review [0] to understand better motivation
for this series.

Thanks in advance for your feedback.

Best regards,
    Sergio Paracuellos

[0]: https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20210926145931.14603-3-sergio.paracuellos@gmail.com/ 

Sergio Paracuellos (4):
  dt-bindings: reset: add dt binding header for Mediatek MT7621 resets
  dt-bindings: clock: mediatek,mt7621-sysc: add '#reset-cells' property
  clk: ralink: make system controller node a reset provider
  staging: mt7621-dts: align resets with binding documentation

 .../bindings/clock/mediatek,mt7621-sysc.yaml  | 12 +++
 drivers/clk/ralink/clk-mt7621.c               | 79 +++++++++++++++++++
 drivers/staging/mt7621-dts/mt7621.dtsi        | 27 +++----
 include/dt-bindings/reset/mt7621-reset.h      | 37 +++++++++
 4 files changed, 140 insertions(+), 15 deletions(-)
 create mode 100644 include/dt-bindings/reset/mt7621-reset.h

-- 
2.33.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/4] dt-bindings: reset: add dt binding header for Mediatek MT7621 resets
  2021-10-06  6:12 [PATCH 0/4] clk: ralink: make system controller a reset provider Sergio Paracuellos
@ 2021-10-06  6:12 ` Sergio Paracuellos
  2021-10-06  6:12 ` [PATCH 2/4] dt-bindings: clock: mediatek,mt7621-sysc: add '#reset-cells' property Sergio Paracuellos
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Sergio Paracuellos @ 2021-10-06  6:12 UTC (permalink / raw)
  To: sboyd
  Cc: linux-clk, gregkh, devicetree, robh+dt, linux-staging, neil,
	linux-kernel, john

Add dt binding header for resets lines in Mediatek MT7621 SoCs.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 include/dt-bindings/reset/mt7621-reset.h | 37 ++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 include/dt-bindings/reset/mt7621-reset.h

diff --git a/include/dt-bindings/reset/mt7621-reset.h b/include/dt-bindings/reset/mt7621-reset.h
new file mode 100644
index 000000000000..7572c6b41453
--- /dev/null
+++ b/include/dt-bindings/reset/mt7621-reset.h
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (c) 2021 Sergio Paracuellos
+ * Author: Sergio Paracuellos <sergio.paracuellos@gmail.com>
+ */
+
+#ifndef DT_BINDING_MT7621_RESET_H
+#define DT_BINDING_MT7621_RESET_H
+
+#define MT7621_RST_SYS		0
+#define MT7621_RST_MCM		2
+#define MT7621_RST_HSDMA	5
+#define MT7621_RST_FE		6
+#define MT7621_RST_SPDIFTX	7
+#define MT7621_RST_TIMER	8
+#define MT7621_RST_INT		9
+#define MT7621_RST_MC		10
+#define MT7621_RST_PCM		11
+#define MT7621_RST_PIO		13
+#define MT7621_RST_GDMA		14
+#define MT7621_RST_NFI		15
+#define MT7621_RST_I2C		16
+#define MT7621_RST_I2S		17
+#define MT7621_RST_SPI		18
+#define MT7621_RST_UART1	19
+#define MT7621_RST_UART2	20
+#define MT7621_RST_UART3	21
+#define MT7621_RST_ETH		23
+#define MT7621_RST_PCIE0	24
+#define MT7621_RST_PCIE1	25
+#define MT7621_RST_PCIE2	26
+#define MT7621_RST_AUX_STCK	28
+#define MT7621_RST_CRYPTO	29
+#define MT7621_RST_SDXC		30
+#define MT7621_RST_PPE		31
+
+#endif /* DT_BINDING_MT7621_RESET_H */
-- 
2.33.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/4] dt-bindings: clock: mediatek,mt7621-sysc: add '#reset-cells' property
  2021-10-06  6:12 [PATCH 0/4] clk: ralink: make system controller a reset provider Sergio Paracuellos
  2021-10-06  6:12 ` [PATCH 1/4] dt-bindings: reset: add dt binding header for Mediatek MT7621 resets Sergio Paracuellos
@ 2021-10-06  6:12 ` Sergio Paracuellos
  2021-10-06  6:12 ` [PATCH 3/4] clk: ralink: make system controller node a reset provider Sergio Paracuellos
  2021-10-06  6:12 ` [PATCH 4/4] staging: mt7621-dts: align resets with binding documentation Sergio Paracuellos
  3 siblings, 0 replies; 9+ messages in thread
From: Sergio Paracuellos @ 2021-10-06  6:12 UTC (permalink / raw)
  To: sboyd
  Cc: linux-clk, gregkh, devicetree, robh+dt, linux-staging, neil,
	linux-kernel, john

Make system controller a reset provider for all the peripherals in the
MT7621 SoC adding '#reset-cells' property.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 .../bindings/clock/mediatek,mt7621-sysc.yaml         | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/mediatek,mt7621-sysc.yaml b/Documentation/devicetree/bindings/clock/mediatek,mt7621-sysc.yaml
index 915f84efd763..0c0b0ae5e2ac 100644
--- a/Documentation/devicetree/bindings/clock/mediatek,mt7621-sysc.yaml
+++ b/Documentation/devicetree/bindings/clock/mediatek,mt7621-sysc.yaml
@@ -22,6 +22,11 @@ description: |
 
   The clocks are provided inside a system controller node.
 
+  This node is also a reset provider for all the peripherals.
+
+  Reset related bits are defined in:
+  [2]: <include/dt-bindings/reset/mt7621-reset.h>.
+
 properties:
   compatible:
     items:
@@ -37,6 +42,12 @@ properties:
       clocks.
     const: 1
 
+  "#reset-cells":
+    description:
+      The first cell indicates the reset bit within the register, see
+      [2] for available resets.
+    const: 1
+
   ralink,memctl:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
@@ -61,6 +72,7 @@ examples:
       compatible = "mediatek,mt7621-sysc", "syscon";
       reg = <0x0 0x100>;
       #clock-cells = <1>;
+      #reset-cells = <1>;
       ralink,memctl = <&memc>;
       clock-output-names = "xtal", "cpu", "bus",
                            "50m", "125m", "150m",
-- 
2.33.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/4] clk: ralink: make system controller node a reset provider
  2021-10-06  6:12 [PATCH 0/4] clk: ralink: make system controller a reset provider Sergio Paracuellos
  2021-10-06  6:12 ` [PATCH 1/4] dt-bindings: reset: add dt binding header for Mediatek MT7621 resets Sergio Paracuellos
  2021-10-06  6:12 ` [PATCH 2/4] dt-bindings: clock: mediatek,mt7621-sysc: add '#reset-cells' property Sergio Paracuellos
@ 2021-10-06  6:12 ` Sergio Paracuellos
  2021-10-06  8:29   ` Dan Carpenter
  2021-10-06  6:12 ` [PATCH 4/4] staging: mt7621-dts: align resets with binding documentation Sergio Paracuellos
  3 siblings, 1 reply; 9+ messages in thread
From: Sergio Paracuellos @ 2021-10-06  6:12 UTC (permalink / raw)
  To: sboyd
  Cc: linux-clk, gregkh, devicetree, robh+dt, linux-staging, neil,
	linux-kernel, john

MT7621 system controller node is already providing the clocks for the whole
system but must also serve as a reset provider. Hence, add reset controller
related code to the clock driver itself.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/clk/ralink/clk-mt7621.c | 79 +++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/drivers/clk/ralink/clk-mt7621.c b/drivers/clk/ralink/clk-mt7621.c
index a2c045390f00..67ccc9582c46 100644
--- a/drivers/clk/ralink/clk-mt7621.c
+++ b/drivers/clk/ralink/clk-mt7621.c
@@ -11,14 +11,17 @@
 #include <linux/mfd/syscon.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/reset-controller.h>
 #include <linux/slab.h>
 #include <dt-bindings/clock/mt7621-clk.h>
+#include <dt-bindings/reset/mt7621-reset.h>
 
 /* Configuration registers */
 #define SYSC_REG_SYSTEM_CONFIG0         0x10
 #define SYSC_REG_SYSTEM_CONFIG1         0x14
 #define SYSC_REG_CLKCFG0		0x2c
 #define SYSC_REG_CLKCFG1		0x30
+#define SYSC_REG_RESET_CTRL		0x34
 #define SYSC_REG_CUR_CLK_STS		0x44
 #define MEMC_REG_CPU_PLL		0x648
 
@@ -398,6 +401,76 @@ static void __init mt7621_clk_init(struct device_node *node)
 }
 CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init);
 
+struct mt7621_rst {
+	struct reset_controller_dev rcdev;
+	struct regmap *sysc;
+};
+
+static inline struct mt7621_rst *to_mt7621_rst(struct reset_controller_dev *dev)
+{
+	return container_of(dev, struct mt7621_rst, rcdev);
+}
+
+static int mt7621_assert_device(struct reset_controller_dev *rcdev,
+				unsigned long id)
+{
+	struct mt7621_rst *data = to_mt7621_rst(rcdev);
+	struct regmap *sysc = data->sysc;
+
+	if (id == MT7621_RST_SYS)
+		return -1;
+
+	return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), BIT(id));
+}
+
+static int mt7621_deassert_device(struct reset_controller_dev *rcdev,
+				  unsigned long id)
+{
+	struct mt7621_rst *data = to_mt7621_rst(rcdev);
+	struct regmap *sysc = data->sysc;
+
+	if (id == MT7621_RST_SYS)
+		return -1;
+
+	return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), 0);
+}
+
+static int mt7621_reset_device(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	int ret;
+
+	ret = mt7621_assert_device(rcdev, id);
+	if (ret < 0)
+		return ret;
+
+	return mt7621_deassert_device(rcdev, id);
+}
+
+static const struct reset_control_ops reset_ops = {
+	.reset = mt7621_reset_device,
+	.assert = mt7621_assert_device,
+	.deassert = mt7621_deassert_device
+};
+
+static int mt7621_reset_init(struct device *dev, struct regmap *sysc)
+{
+	struct mt7621_rst *rst_data;
+
+	rst_data = kzalloc(sizeof(*rst_data), GFP_KERNEL);
+	if (!rst_data)
+		return -ENOMEM;
+
+	rst_data->sysc = sysc;
+	rst_data->rcdev.ops = &reset_ops;
+	rst_data->rcdev.owner = THIS_MODULE;
+	rst_data->rcdev.nr_resets = 32;
+	rst_data->rcdev.of_reset_n_cells = 1;
+	rst_data->rcdev.of_node = dev_of_node(dev);
+
+	return devm_reset_controller_register(dev, &rst_data->rcdev);
+}
+
 static int mt7621_clk_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
@@ -424,6 +497,12 @@ static int mt7621_clk_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = mt7621_reset_init(dev, priv->sysc);
+	if (ret) {
+		pr_err("Could not init reset controller\n");
+		return ret;
+	}
+
 	count = ARRAY_SIZE(mt7621_clks_base) +
 		ARRAY_SIZE(mt7621_fixed_clks) + ARRAY_SIZE(mt7621_gates);
 	clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, count),
-- 
2.33.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 4/4] staging: mt7621-dts: align resets with binding documentation
  2021-10-06  6:12 [PATCH 0/4] clk: ralink: make system controller a reset provider Sergio Paracuellos
                   ` (2 preceding siblings ...)
  2021-10-06  6:12 ` [PATCH 3/4] clk: ralink: make system controller node a reset provider Sergio Paracuellos
@ 2021-10-06  6:12 ` Sergio Paracuellos
  3 siblings, 0 replies; 9+ messages in thread
From: Sergio Paracuellos @ 2021-10-06  6:12 UTC (permalink / raw)
  To: sboyd
  Cc: linux-clk, gregkh, devicetree, robh+dt, linux-staging, neil,
	linux-kernel, john

Binding documentation for compatible 'mediatek,mt7621-sysc' has been updated
to be used as a reset provider. Align reset related bits and system controller
node with binding documentation along the dtsi file.

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
---
 drivers/staging/mt7621-dts/mt7621.dtsi | 27 ++++++++++++--------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi
index 719ef28171f4..72b99d8b4647 100644
--- a/drivers/staging/mt7621-dts/mt7621.dtsi
+++ b/drivers/staging/mt7621-dts/mt7621.dtsi
@@ -1,6 +1,7 @@
 #include <dt-bindings/interrupt-controller/mips-gic.h>
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/clock/mt7621-clk.h>
+#include <dt-bindings/reset/mt7621-reset.h>
 
 / {
 	#address-cells = <1>;
@@ -59,6 +60,7 @@ sysc: syscon@0 {
 			compatible = "mediatek,mt7621-sysc", "syscon";
 			reg = <0x0 0x100>;
 			#clock-cells = <1>;
+			#reset-cells = <1>;
 			ralink,memctl = <&memc>;
 			clock-output-names = "xtal", "cpu", "bus",
 					     "50m", "125m", "150m",
@@ -88,7 +90,7 @@ i2c: i2c@900 {
 
 			clocks = <&sysc MT7621_CLK_I2C>;
 			clock-names = "i2c";
-			resets = <&rstctrl 16>;
+			resets = <&sysc MT7621_RST_I2C>;
 			reset-names = "i2c";
 
 			#address-cells = <1>;
@@ -106,7 +108,7 @@ i2s: i2s@a00 {
 
 			clocks = <&sysc MT7621_CLK_I2S>;
 			clock-names = "i2s";
-			resets = <&rstctrl 17>;
+			resets = <&sysc MT7621_RST_I2S>;
 			reset-names = "i2s";
 
 			interrupt-parent = <&gic>;
@@ -151,7 +153,7 @@ spi0: spi@b00 {
 			clocks = <&sysc MT7621_CLK_SPI>;
 			clock-names = "spi";
 
-			resets = <&rstctrl 18>;
+			resets = <&sysc MT7621_RST_SPI>;
 			reset-names = "spi";
 
 			#address-cells = <1>;
@@ -167,7 +169,7 @@ gdma: gdma@2800 {
 
 			clocks = <&sysc MT7621_CLK_GDMA>;
 			clock-names = "gdma";
-			resets = <&rstctrl 14>;
+			resets = <&sysc MT7621_RST_GDMA>;
 			reset-names = "dma";
 
 			interrupt-parent = <&gic>;
@@ -186,7 +188,7 @@ hsdma: hsdma@7000 {
 
 			clocks = <&sysc MT7621_CLK_HSDMA>;
 			clock-names = "hsdma";
-			resets = <&rstctrl 5>;
+			resets = <&sysc MT7621_RST_HSDMA>;
 			reset-names = "hsdma";
 
 			interrupt-parent = <&gic>;
@@ -286,11 +288,6 @@ pinmux {
 		};
 	};
 
-	rstctrl: rstctrl {
-		compatible = "ralink,rt2880-reset";
-		#reset-cells = <1>;
-	};
-
 	sdhci: sdhci@1E130000 {
 		status = "disabled";
 
@@ -383,7 +380,7 @@ ethernet: ethernet@1e100000 {
 		#address-cells = <1>;
 		#size-cells = <0>;
 
-		resets = <&rstctrl 6 &rstctrl 23>;
+		resets = <&sysc MT7621_CLK_FE &sysc MT7621_CLK_ETH>;
 		reset-names = "fe", "eth";
 
 		interrupt-parent = <&gic>;
@@ -428,7 +425,7 @@ switch0: switch0@0 {
 				#size-cells = <0>;
 				reg = <0>;
 				mediatek,mcm;
-				resets = <&rstctrl 2>;
+				resets = <&sysc MT7621_RST_MCM>;
 				reset-names = "mcm";
 				interrupt-controller;
 				#interrupt-cells = <1>;
@@ -514,7 +511,7 @@ pcie@0,0 {
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 0>;
 			interrupt-map = <0 0 0 0 &gic GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>;
-			resets = <&rstctrl 24>;
+			resets = <&sysc MT7621_RST_PCIE0>;
 			clocks = <&sysc MT7621_CLK_PCIE0>;
 			phys = <&pcie0_phy 1>;
 			phy-names = "pcie-phy0";
@@ -529,7 +526,7 @@ pcie@1,0 {
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 0>;
 			interrupt-map = <0 0 0 0 &gic GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>;
-			resets = <&rstctrl 25>;
+			resets = <&sysc MT7621_RST_PCIE1>;
 			clocks = <&sysc MT7621_CLK_PCIE1>;
 			phys = <&pcie0_phy 1>;
 			phy-names = "pcie-phy1";
@@ -544,7 +541,7 @@ pcie@2,0 {
 			#interrupt-cells = <1>;
 			interrupt-map-mask = <0 0 0 0>;
 			interrupt-map = <0 0 0 0 &gic GIC_SHARED 25 IRQ_TYPE_LEVEL_HIGH>;
-			resets = <&rstctrl 26>;
+			resets = <&sysc MT7621_RST_PCIE2>;
 			clocks = <&sysc MT7621_CLK_PCIE2>;
 			phys = <&pcie2_phy 0>;
 			phy-names = "pcie-phy2";
-- 
2.33.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4] clk: ralink: make system controller node a reset provider
  2021-10-06  6:12 ` [PATCH 3/4] clk: ralink: make system controller node a reset provider Sergio Paracuellos
@ 2021-10-06  8:29   ` Dan Carpenter
  2021-10-06 10:02     ` Sergio Paracuellos
  0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2021-10-06  8:29 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: sboyd, linux-clk, gregkh, devicetree, robh+dt, linux-staging,
	neil, linux-kernel, john

On Wed, Oct 06, 2021 at 08:12:03AM +0200, Sergio Paracuellos wrote:
> @@ -398,6 +401,76 @@ static void __init mt7621_clk_init(struct device_node *node)
>  }
>  CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init);
>  
> +struct mt7621_rst {
> +	struct reset_controller_dev rcdev;
> +	struct regmap *sysc;
> +};
> +
> +static inline struct mt7621_rst *to_mt7621_rst(struct reset_controller_dev *dev)

No need to mark this as inline.  The compiler should do it automatically
or it will ignore the inline.

> +{
> +	return container_of(dev, struct mt7621_rst, rcdev);
> +}
> +
> +static int mt7621_assert_device(struct reset_controller_dev *rcdev,
> +				unsigned long id)
> +{
> +	struct mt7621_rst *data = to_mt7621_rst(rcdev);
> +	struct regmap *sysc = data->sysc;
> +
> +	if (id == MT7621_RST_SYS)
> +		return -1;

Please, return proper error codes.

> +
> +	return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), BIT(id));
> +}
> +
> +static int mt7621_deassert_device(struct reset_controller_dev *rcdev,
> +				  unsigned long id)
> +{
> +	struct mt7621_rst *data = to_mt7621_rst(rcdev);
> +	struct regmap *sysc = data->sysc;
> +
> +	if (id == MT7621_RST_SYS)
> +		return -1;

Here too.

> +
> +	return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), 0);
> +}
> +
> +static int mt7621_reset_device(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	int ret;
> +
> +	ret = mt7621_assert_device(rcdev, id);
> +	if (ret < 0)
> +		return ret;
> +
> +	return mt7621_deassert_device(rcdev, id);
> +}
> +
> +static const struct reset_control_ops reset_ops = {
> +	.reset = mt7621_reset_device,
> +	.assert = mt7621_assert_device,
> +	.deassert = mt7621_deassert_device
> +};
> +
> +static int mt7621_reset_init(struct device *dev, struct regmap *sysc)
> +{
> +	struct mt7621_rst *rst_data;
> +
> +	rst_data = kzalloc(sizeof(*rst_data), GFP_KERNEL);


Can we use devm_ to allocate this or do we need to clean up if
devm_reset_controller_register() fails?  Also a free in the release
function I suppose.  (Please, use devm_).


> +	if (!rst_data)
> +		return -ENOMEM;
> +
> +	rst_data->sysc = sysc;
> +	rst_data->rcdev.ops = &reset_ops;
> +	rst_data->rcdev.owner = THIS_MODULE;
> +	rst_data->rcdev.nr_resets = 32;
> +	rst_data->rcdev.of_reset_n_cells = 1;
> +	rst_data->rcdev.of_node = dev_of_node(dev);
> +
> +	return devm_reset_controller_register(dev, &rst_data->rcdev);
> +}


regards,
dan carpenter


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4] clk: ralink: make system controller node a reset provider
  2021-10-06  8:29   ` Dan Carpenter
@ 2021-10-06 10:02     ` Sergio Paracuellos
  2021-10-06 10:14       ` Dan Carpenter
  0 siblings, 1 reply; 9+ messages in thread
From: Sergio Paracuellos @ 2021-10-06 10:02 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Stephen Boyd, open list:COMMON CLK FRAMEWORK, Greg KH,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, linux-staging, NeilBrown, linux-kernel,
	John Crispin

Hi Dan,

Thanks for the review. Comments below.

On Wed, Oct 6, 2021 at 10:29 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, Oct 06, 2021 at 08:12:03AM +0200, Sergio Paracuellos wrote:
> > @@ -398,6 +401,76 @@ static void __init mt7621_clk_init(struct device_node *node)
> >  }
> >  CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init);
> >
> > +struct mt7621_rst {
> > +     struct reset_controller_dev rcdev;
> > +     struct regmap *sysc;
> > +};
> > +
> > +static inline struct mt7621_rst *to_mt7621_rst(struct reset_controller_dev *dev)
>
> No need to mark this as inline.  The compiler should do it automatically
> or it will ignore the inline.

Ok, I have other functions to_* with the same inline syntax, that's
why I have added also here. I think I will maintain it to be coherent
and can be removed afterwards with another patch not belonging to this
series.

>
> > +{
> > +     return container_of(dev, struct mt7621_rst, rcdev);
> > +}
> > +
> > +static int mt7621_assert_device(struct reset_controller_dev *rcdev,
> > +                             unsigned long id)
> > +{
> > +     struct mt7621_rst *data = to_mt7621_rst(rcdev);
> > +     struct regmap *sysc = data->sysc;
> > +
> > +     if (id == MT7621_RST_SYS)
> > +             return -1;
>
> Please, return proper error codes.

Current code at 'reset.c' of the arch was returning -1 in this case. I
guess it is better to change it to -EINVAL.

>
> > +
> > +     return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), BIT(id));
> > +}
> > +
> > +static int mt7621_deassert_device(struct reset_controller_dev *rcdev,
> > +                               unsigned long id)
> > +{
> > +     struct mt7621_rst *data = to_mt7621_rst(rcdev);
> > +     struct regmap *sysc = data->sysc;
> > +
> > +     if (id == MT7621_RST_SYS)
> > +             return -1;
>
> Here too.

Will change to -EINVAL.

>
> > +
> > +     return regmap_update_bits(sysc, SYSC_REG_RESET_CTRL, BIT(id), 0);
> > +}
> > +
> > +static int mt7621_reset_device(struct reset_controller_dev *rcdev,
> > +                            unsigned long id)
> > +{
> > +     int ret;
> > +
> > +     ret = mt7621_assert_device(rcdev, id);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     return mt7621_deassert_device(rcdev, id);
> > +}
> > +
> > +static const struct reset_control_ops reset_ops = {
> > +     .reset = mt7621_reset_device,
> > +     .assert = mt7621_assert_device,
> > +     .deassert = mt7621_deassert_device
> > +};
> > +
> > +static int mt7621_reset_init(struct device *dev, struct regmap *sysc)
> > +{
> > +     struct mt7621_rst *rst_data;
> > +
> > +     rst_data = kzalloc(sizeof(*rst_data), GFP_KERNEL);
>
>
> Can we use devm_ to allocate this or do we need to clean up if
> devm_reset_controller_register() fails?  Also a free in the release
> function I suppose.  (Please, use devm_).

True, yes we can use devm_ for this. Will change it, thanks.

>
>
> > +     if (!rst_data)
> > +             return -ENOMEM;
> > +
> > +     rst_data->sysc = sysc;
> > +     rst_data->rcdev.ops = &reset_ops;
> > +     rst_data->rcdev.owner = THIS_MODULE;
> > +     rst_data->rcdev.nr_resets = 32;
> > +     rst_data->rcdev.of_reset_n_cells = 1;
> > +     rst_data->rcdev.of_node = dev_of_node(dev);
> > +
> > +     return devm_reset_controller_register(dev, &rst_data->rcdev);
> > +}
>
>
> regards,
> dan carpenter
>
I will properly take into account your comments and send v2.

Thanks,
     Sergio Paracuellos

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4] clk: ralink: make system controller node a reset provider
  2021-10-06 10:02     ` Sergio Paracuellos
@ 2021-10-06 10:14       ` Dan Carpenter
  2021-10-06 10:20         ` Sergio Paracuellos
  0 siblings, 1 reply; 9+ messages in thread
From: Dan Carpenter @ 2021-10-06 10:14 UTC (permalink / raw)
  To: Sergio Paracuellos
  Cc: Stephen Boyd, open list:COMMON CLK FRAMEWORK, Greg KH,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, linux-staging, NeilBrown, linux-kernel,
	John Crispin

On Wed, Oct 06, 2021 at 12:02:12PM +0200, Sergio Paracuellos wrote:
> Hi Dan,
> 
> Thanks for the review. Comments below.
> 
> On Wed, Oct 6, 2021 at 10:29 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> >
> > On Wed, Oct 06, 2021 at 08:12:03AM +0200, Sergio Paracuellos wrote:
> > > @@ -398,6 +401,76 @@ static void __init mt7621_clk_init(struct device_node *node)
> > >  }
> > >  CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init);
> > >
> > > +struct mt7621_rst {
> > > +     struct reset_controller_dev rcdev;
> > > +     struct regmap *sysc;
> > > +};
> > > +
> > > +static inline struct mt7621_rst *to_mt7621_rst(struct reset_controller_dev *dev)
> >
> > No need to mark this as inline.  The compiler should do it automatically
> > or it will ignore the inline.
> 
> Ok, I have other functions to_* with the same inline syntax, that's
> why I have added also here. I think I will maintain it to be coherent
> and can be removed afterwards with another patch not belonging to this
> series.

Consistency is never an important goal.  It's better to be different
than to be wrong.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/4] clk: ralink: make system controller node a reset provider
  2021-10-06 10:14       ` Dan Carpenter
@ 2021-10-06 10:20         ` Sergio Paracuellos
  0 siblings, 0 replies; 9+ messages in thread
From: Sergio Paracuellos @ 2021-10-06 10:20 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Stephen Boyd, open list:COMMON CLK FRAMEWORK, Greg KH,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Rob Herring, linux-staging, NeilBrown, linux-kernel,
	John Crispin

On Wed, Oct 6, 2021 at 12:15 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Wed, Oct 06, 2021 at 12:02:12PM +0200, Sergio Paracuellos wrote:
> > Hi Dan,
> >
> > Thanks for the review. Comments below.
> >
> > On Wed, Oct 6, 2021 at 10:29 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Wed, Oct 06, 2021 at 08:12:03AM +0200, Sergio Paracuellos wrote:
> > > > @@ -398,6 +401,76 @@ static void __init mt7621_clk_init(struct device_node *node)
> > > >  }
> > > >  CLK_OF_DECLARE_DRIVER(mt7621_clk, "mediatek,mt7621-sysc", mt7621_clk_init);
> > > >
> > > > +struct mt7621_rst {
> > > > +     struct reset_controller_dev rcdev;
> > > > +     struct regmap *sysc;
> > > > +};
> > > > +
> > > > +static inline struct mt7621_rst *to_mt7621_rst(struct reset_controller_dev *dev)
> > >
> > > No need to mark this as inline.  The compiler should do it automatically
> > > or it will ignore the inline.
> >
> > Ok, I have other functions to_* with the same inline syntax, that's
> > why I have added also here. I think I will maintain it to be coherent
> > and can be removed afterwards with another patch not belonging to this
> > series.
>
> Consistency is never an important goal.  It's better to be different
> than to be wrong.

Pretty clear, thanks. Will change this also, then.

>
> regards,
> dan carpenter
>

Best regards,
    Sergio Paracuellos

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-10-06 10:20 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-06  6:12 [PATCH 0/4] clk: ralink: make system controller a reset provider Sergio Paracuellos
2021-10-06  6:12 ` [PATCH 1/4] dt-bindings: reset: add dt binding header for Mediatek MT7621 resets Sergio Paracuellos
2021-10-06  6:12 ` [PATCH 2/4] dt-bindings: clock: mediatek,mt7621-sysc: add '#reset-cells' property Sergio Paracuellos
2021-10-06  6:12 ` [PATCH 3/4] clk: ralink: make system controller node a reset provider Sergio Paracuellos
2021-10-06  8:29   ` Dan Carpenter
2021-10-06 10:02     ` Sergio Paracuellos
2021-10-06 10:14       ` Dan Carpenter
2021-10-06 10:20         ` Sergio Paracuellos
2021-10-06  6:12 ` [PATCH 4/4] staging: mt7621-dts: align resets with binding documentation Sergio Paracuellos

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).