* [PATCH 0/3] Extend Hisilicon reset type
@ 2019-12-02 14:45 Jun Nie
2019-12-02 14:45 ` [PATCH 1/3] dt-bindings: clock: Update Hisilicon reset doc Jun Nie
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Jun Nie @ 2019-12-02 14:45 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, xuwei5, p.zabel,
opensource, swinslow, jun.nie, allison, yuehaibing, tglx,
linux-clk, devicetree, linux-arm-kernel, xuejiancheng
Extend Hisilicon reset type to support combination of set/clear,
assert/deassert and poll/non-poll.
Jun Nie (3):
dt-bindings: clock: Update Hisilicon reset doc
reset: hisilicon: Extend reset operation type
ARM: dts: Update reset for hi3519 and hi3798cv200
.../devicetree/bindings/clock/hisi-crg.txt | 12 ++--
arch/arm/boot/dts/hi3519.dtsi | 2 +-
.../arm64/boot/dts/hisilicon/hi3798cv200.dtsi | 47 ++++++++-------
drivers/clk/hisilicon/reset.c | 58 +++++++++++++++++--
include/dt-bindings/reset/hisilicon-resets.h | 28 +++++++++
5 files changed, 116 insertions(+), 31 deletions(-)
create mode 100644 include/dt-bindings/reset/hisilicon-resets.h
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] dt-bindings: clock: Update Hisilicon reset doc
2019-12-02 14:45 [PATCH 0/3] Extend Hisilicon reset type Jun Nie
@ 2019-12-02 14:45 ` Jun Nie
2019-12-02 17:04 ` Philipp Zabel
2019-12-02 14:45 ` [PATCH 2/3] reset: hisilicon: Extend reset operation type Jun Nie
2019-12-02 14:45 ` [PATCH 3/3] ARM: dts: Update reset for hi3519 and hi3798cv200 Jun Nie
2 siblings, 1 reply; 11+ messages in thread
From: Jun Nie @ 2019-12-02 14:45 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, xuwei5, p.zabel,
opensource, swinslow, jun.nie, allison, yuehaibing, tglx,
linux-clk, devicetree, linux-arm-kernel, xuejiancheng
Document the update of Hisilicon reset operation extension.
Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
.../devicetree/bindings/clock/hisi-crg.txt | 12 ++++----
include/dt-bindings/reset/hisilicon-resets.h | 28 +++++++++++++++++++
2 files changed, 35 insertions(+), 5 deletions(-)
create mode 100644 include/dt-bindings/reset/hisilicon-resets.h
diff --git a/Documentation/devicetree/bindings/clock/hisi-crg.txt b/Documentation/devicetree/bindings/clock/hisi-crg.txt
index cc60b3d423f3..fd8b0a964806 100644
--- a/Documentation/devicetree/bindings/clock/hisi-crg.txt
+++ b/Documentation/devicetree/bindings/clock/hisi-crg.txt
@@ -26,19 +26,21 @@ to specify the clock which they consume.
All these identifier could be found in <dt-bindings/clock/hi3519-clock.h>.
-- #reset-cells: should be 2.
+- #reset-cells: should be 3.
A reset signal can be controlled by writing a bit register in the CRG module.
-The reset specifier consists of two cells. The first cell represents the
+The reset specifier consists of three cells. The first cell represents the
register offset relative to the base address. The second cell represents the
-bit index in the register.
+bit index in the register. The third represent the flags to operation type.
+
+All reset flags could be found in <dt-bindings/reset/hisilicon-resets.h>
Example: CRG nodes
CRG: clock-reset-controller@12010000 {
compatible = "hisilicon,hi3519-crg";
reg = <0x12010000 0x10000>;
#clock-cells = <1>;
- #reset-cells = <2>;
+ #reset-cells = <3>;
};
Example: consumer nodes
@@ -46,5 +48,5 @@ i2c0: i2c@12110000 {
compatible = "hisilicon,hi3519-i2c";
reg = <0x12110000 0x1000>;
clocks = <&CRG HI3519_I2C0_RST>;
- resets = <&CRG 0xe4 0>;
+ resets = <&CRG 0xe4 0 (HISI_ASSERT_SET | HISI_DEASSERT_CLEAR)>;
};
diff --git a/include/dt-bindings/reset/hisilicon-resets.h b/include/dt-bindings/reset/hisilicon-resets.h
new file mode 100644
index 000000000000..983e42a0c318
--- /dev/null
+++ b/include/dt-bindings/reset/hisilicon-resets.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Hisilicon Reset definitions
+ *
+ * Copyright (c) 2019 HiSilicon Technologies Co., Ltd.
+ */
+
+#ifndef __DT_BINDINGS_RESET_HISILICON_H__
+#define __DT_BINDINGS_RESET_HISILICON_H__
+
+/*
+ * The reset does not support the feature and corresponding
+ * values are not valid
+ */
+#define HISI_ASSERT_NONE (1 << 0)
+#define HISI_DEASSERT_NONE (1 << 1)
+
+/* When set this function is activated by polling/setting/clearing this bit */
+#define HISI_ASSERT_SET (1 << 2)
+#define HISI_DEASSERT_SET (1 << 3)
+#define HISI_ASSERT_CLEAR (0 << 4)
+#define HISI_DEASSERT_CLEAR (0 << 5)
+#define HISI_ASSERT_POLL (0 << 6)
+#define HISI_DEASSERT_POLL (0 << 7)
+
+#define HISI_RESET_DEFAULT (HISI_ASSERT_SET | HISI_DEASSERT_CLEAR)
+
+#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] reset: hisilicon: Extend reset operation type
2019-12-02 14:45 [PATCH 0/3] Extend Hisilicon reset type Jun Nie
2019-12-02 14:45 ` [PATCH 1/3] dt-bindings: clock: Update Hisilicon reset doc Jun Nie
@ 2019-12-02 14:45 ` Jun Nie
2019-12-02 17:04 ` Philipp Zabel
2019-12-02 14:45 ` [PATCH 3/3] ARM: dts: Update reset for hi3519 and hi3798cv200 Jun Nie
2 siblings, 1 reply; 11+ messages in thread
From: Jun Nie @ 2019-12-02 14:45 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, xuwei5, p.zabel,
opensource, swinslow, jun.nie, allison, yuehaibing, tglx,
linux-clk, devicetree, linux-arm-kernel, xuejiancheng
Extend reset operations to support combination of three type flags:
ASSERT/DEASSERT SET/CLEAR POLL.
Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
drivers/clk/hisilicon/reset.c | 58 ++++++++++++++++++++++++++++++++---
1 file changed, 53 insertions(+), 5 deletions(-)
diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
index 93cee17db8b1..de7d186b0894 100644
--- a/drivers/clk/hisilicon/reset.c
+++ b/drivers/clk/hisilicon/reset.c
@@ -2,20 +2,25 @@
/*
* Hisilicon Reset Controller Driver
*
- * Copyright (c) 2015-2016 HiSilicon Technologies Co., Ltd.
+ * Copyright (c) 2015-2019 HiSilicon Technologies Co., Ltd.
*/
#include <linux/io.h>
+#include <linux/iopoll.h>
#include <linux/of_address.h>
#include <linux/platform_device.h>
#include <linux/reset-controller.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
+
+#include <dt-bindings/reset/hisilicon-resets.h>
#include "reset.h"
#define HISI_RESET_BIT_MASK 0x1f
#define HISI_RESET_OFFSET_SHIFT 8
#define HISI_RESET_OFFSET_MASK 0xffff00
+#define HISI_RESET_FLAG_SHIFT 24
+#define HISI_RESET_FLAG_MASK 0xff000000
struct hisi_reset_controller {
spinlock_t lock;
@@ -30,14 +35,17 @@ struct hisi_reset_controller {
static int hisi_reset_of_xlate(struct reset_controller_dev *rcdev,
const struct of_phandle_args *reset_spec)
{
+ unsigned long flags;
u32 offset;
u8 bit;
+ flags = (reset_spec->args[2] << HISI_RESET_FLAG_SHIFT)
+ & HISI_RESET_FLAG_MASK;
offset = (reset_spec->args[0] << HISI_RESET_OFFSET_SHIFT)
& HISI_RESET_OFFSET_MASK;
bit = reset_spec->args[1] & HISI_RESET_BIT_MASK;
- return (offset | bit);
+ return (flags | offset | bit);
}
static int hisi_reset_assert(struct reset_controller_dev *rcdev,
@@ -48,13 +56,33 @@ static int hisi_reset_assert(struct reset_controller_dev *rcdev,
u32 offset, reg;
u8 bit;
+ flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
+ if (flags & HISI_ASSERT_NONE)
+ return -ENOTSUPP; /* assert not supported for this reset */
+
offset = (id & HISI_RESET_OFFSET_MASK) >> HISI_RESET_OFFSET_SHIFT;
bit = id & HISI_RESET_BIT_MASK;
+ pr_devel("%s %s to %s 0x%x:bit[%d]\n", __func__,
+ flags & HISI_ASSERT_POLL ? "poll" : "",
+ flags & HISI_ASSERT_SET ? "set":"clear", offset, bit);
+
+ if (flags & HISI_ASSERT_POLL) {
+ if (flags & HISI_ASSERT_SET)
+ return readl_poll_timeout(rstc->membase + offset,
+ reg, reg & BIT(bit), 0, 5000);
+ else
+ return readl_poll_timeout(rstc->membase + offset,
+ reg, !(reg & BIT(bit)),
+ 0, 5000);
+ }
+
spin_lock_irqsave(&rstc->lock, flags);
reg = readl(rstc->membase + offset);
- writel(reg | BIT(bit), rstc->membase + offset);
+ /* Default is setting to assert for no flag case. */
+ reg = (flags & HISI_ASSERT_CLEAR) ? reg & ~BIT(bit) : reg | BIT(bit);
+ writel(reg, rstc->membase + offset);
spin_unlock_irqrestore(&rstc->lock, flags);
@@ -69,13 +97,33 @@ static int hisi_reset_deassert(struct reset_controller_dev *rcdev,
u32 offset, reg;
u8 bit;
+ flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
+ if (flags & HISI_DEASSERT_NONE)
+ return -ENOTSUPP; /* deassert not supported for this reset */
+
offset = (id & HISI_RESET_OFFSET_MASK) >> HISI_RESET_OFFSET_SHIFT;
bit = id & HISI_RESET_BIT_MASK;
+ pr_devel("%s %s to %s 0x%x:bit[%d]\n", __func__,
+ flags & HISI_DEASSERT_POLL ? "poll" : "",
+ flags & HISI_DEASSERT_SET ? "clear":"set", offset, bit);
+
+ if (flags & HISI_DEASSERT_POLL) {
+ if (flags & HISI_DEASSERT_SET)
+ return readl_poll_timeout(rstc->membase + offset,
+ reg, reg & BIT(bit), 0, 5000);
+ else
+ return readl_poll_timeout(rstc->membase + offset,
+ reg, !(reg & BIT(bit)),
+ 0, 5000);
+ }
+
spin_lock_irqsave(&rstc->lock, flags);
reg = readl(rstc->membase + offset);
- writel(reg & ~BIT(bit), rstc->membase + offset);
+ /* Default is clearing to deasseart for no flag case. */
+ reg = (flags & HISI_DEASSERT_SET) ? reg | BIT(bit) : reg & ~BIT(bit);
+ writel(reg, rstc->membase + offset);
spin_unlock_irqrestore(&rstc->lock, flags);
@@ -103,7 +151,7 @@ struct hisi_reset_controller *hisi_reset_init(struct platform_device *pdev)
rstc->rcdev.owner = THIS_MODULE;
rstc->rcdev.ops = &hisi_reset_ops;
rstc->rcdev.of_node = pdev->dev.of_node;
- rstc->rcdev.of_reset_n_cells = 2;
+ rstc->rcdev.of_reset_n_cells = 3;
rstc->rcdev.of_xlate = hisi_reset_of_xlate;
reset_controller_register(&rstc->rcdev);
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] ARM: dts: Update reset for hi3519 and hi3798cv200
2019-12-02 14:45 [PATCH 0/3] Extend Hisilicon reset type Jun Nie
2019-12-02 14:45 ` [PATCH 1/3] dt-bindings: clock: Update Hisilicon reset doc Jun Nie
2019-12-02 14:45 ` [PATCH 2/3] reset: hisilicon: Extend reset operation type Jun Nie
@ 2019-12-02 14:45 ` Jun Nie
2 siblings, 0 replies; 11+ messages in thread
From: Jun Nie @ 2019-12-02 14:45 UTC (permalink / raw)
To: mturquette, sboyd, robh+dt, mark.rutland, xuwei5, p.zabel,
opensource, swinslow, jun.nie, allison, yuehaibing, tglx,
linux-clk, devicetree, linux-arm-kernel, xuejiancheng
Update reset for hi3519 and hi3798cv200 as driver is extended to
support configurable reset operation type.
Signed-off-by: Jun Nie <jun.nie@linaro.org>
---
arch/arm/boot/dts/hi3519.dtsi | 2 +-
.../arm64/boot/dts/hisilicon/hi3798cv200.dtsi | 47 +++++++++++--------
2 files changed, 28 insertions(+), 21 deletions(-)
diff --git a/arch/arm/boot/dts/hi3519.dtsi b/arch/arm/boot/dts/hi3519.dtsi
index 410409a0ed66..2335c8443d2d 100644
--- a/arch/arm/boot/dts/hi3519.dtsi
+++ b/arch/arm/boot/dts/hi3519.dtsi
@@ -37,7 +37,7 @@
crg: clock-reset-controller@12010000 {
compatible = "hisilicon,hi3519-crg";
#clock-cells = <1>;
- #reset-cells = <2>;
+ #reset-cells = <3>;
reg = <0x12010000 0x10000>;
};
diff --git a/arch/arm64/boot/dts/hisilicon/hi3798cv200.dtsi b/arch/arm64/boot/dts/hisilicon/hi3798cv200.dtsi
index 13821a0ff524..0a30aaae6bf2 100644
--- a/arch/arm64/boot/dts/hisilicon/hi3798cv200.dtsi
+++ b/arch/arm64/boot/dts/hisilicon/hi3798cv200.dtsi
@@ -9,8 +9,10 @@
#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/phy/phy.h>
+#include <dt-bindings/reset/hisilicon-resets.h>
#include <dt-bindings/reset/ti-syscon.h>
+
/ {
compatible = "hisilicon,hi3798cv200";
interrupt-parent = <&gic>;
@@ -86,7 +88,7 @@
compatible = "hisilicon,hi3798cv200-crg", "syscon", "simple-mfd";
reg = <0x8a22000 0x1000>;
#clock-cells = <1>;
- #reset-cells = <2>;
+ #reset-cells = <3>;
gmacphyrst: reset-controller {
compatible = "ti,syscon-reset";
@@ -103,7 +105,7 @@
compatible = "hisilicon,hi3798cv200-sysctrl", "syscon";
reg = <0x8000000 0x1000>;
#clock-cells = <1>;
- #reset-cells = <2>;
+ #reset-cells = <3>;
};
perictrl: peripheral-controller@8a20000 {
@@ -118,20 +120,22 @@
compatible = "hisilicon,hi3798cv200-usb2-phy";
reg = <0x120 0x4>;
clocks = <&crg HISTB_USB2_PHY1_REF_CLK>;
- resets = <&crg 0xbc 4>;
+ resets = <&crg 0xbc 4 HISI_RESET_DEFAULT>;
#address-cells = <1>;
#size-cells = <0>;
usb2_phy1_port0: phy@0 {
reg = <0>;
#phy-cells = <0>;
- resets = <&crg 0xbc 8>;
+ resets = <&crg 0xbc 8
+ HISI_RESET_DEFAULT>;
};
usb2_phy1_port1: phy@1 {
reg = <1>;
#phy-cells = <0>;
- resets = <&crg 0xbc 9>;
+ resets = <&crg 0xbc 9
+ HISI_RESET_DEFAULT>;
};
};
@@ -139,14 +143,15 @@
compatible = "hisilicon,hi3798cv200-usb2-phy";
reg = <0x124 0x4>;
clocks = <&crg HISTB_USB2_PHY2_REF_CLK>;
- resets = <&crg 0xbc 6>;
+ resets = <&crg 0xbc 6 HISI_RESET_DEFAULT>;
#address-cells = <1>;
#size-cells = <0>;
usb2_phy2_port0: phy@0 {
reg = <0>;
#phy-cells = <0>;
- resets = <&crg 0xbc 10>;
+ resets = <&crg 0xbc 10
+ HISI_RESET_DEFAULT>;
};
};
@@ -155,7 +160,7 @@
reg = <0x850 0x8>;
#phy-cells = <1>;
clocks = <&crg HISTB_COMBPHY0_CLK>;
- resets = <&crg 0x188 4>;
+ resets = <&crg 0x188 4 HISI_RESET_DEFAULT>;
assigned-clocks = <&crg HISTB_COMBPHY0_CLK>;
assigned-clock-rates = <100000000>;
hisilicon,fixed-mode = <PHY_TYPE_USB3>;
@@ -166,7 +171,7 @@
reg = <0x858 0x8>;
#phy-cells = <1>;
clocks = <&crg HISTB_COMBPHY1_CLK>;
- resets = <&crg 0x188 12>;
+ resets = <&crg 0x188 12 HISI_RESET_DEFAULT>;
assigned-clocks = <&crg HISTB_COMBPHY1_CLK>;
assigned-clock-rates = <100000000>;
hisilicon,mode-select-bits = <0x0008 11 (0x3 << 11)>;
@@ -306,7 +311,7 @@
clocks = <&crg HISTB_SDIO0_CIU_CLK>,
<&crg HISTB_SDIO0_BIU_CLK>;
clock-names = "ciu", "biu";
- resets = <&crg 0x9c 4>;
+ resets = <&crg 0x9c 4 HISI_RESET_DEFAULT>;
reset-names = "reset";
status = "disabled";
};
@@ -320,7 +325,7 @@
<&crg HISTB_MMC_SAMPLE_CLK>,
<&crg HISTB_MMC_DRV_CLK>;
clock-names = "ciu", "biu", "ciu-sample", "ciu-drive";
- resets = <&crg 0xa0 4>;
+ resets = <&crg 0xa0 4 HISI_RESET_DEFAULT>;
reset-names = "reset";
status = "disabled";
};
@@ -525,8 +530,8 @@
clocks = <&crg HISTB_ETH0_MAC_CLK>,
<&crg HISTB_ETH0_MACIF_CLK>;
clock-names = "mac_core", "mac_ifc";
- resets = <&crg 0xcc 8>,
- <&crg 0xcc 10>,
+ resets = <&crg 0xcc 8 HISI_RESET_DEFAULT>,
+ <&crg 0xcc 10 HISI_RESET_DEFAULT>,
<&gmacphyrst 0>;
reset-names = "mac_core", "mac_ifc", "phy";
status = "disabled";
@@ -540,8 +545,8 @@
clocks = <&crg HISTB_ETH1_MAC_CLK>,
<&crg HISTB_ETH1_MACIF_CLK>;
clock-names = "mac_core", "mac_ifc";
- resets = <&crg 0xcc 9>,
- <&crg 0xcc 11>,
+ resets = <&crg 0xcc 9 HISI_RESET_DEFAULT>,
+ <&crg 0xcc 11 HISI_RESET_DEFAULT>,
<&gmacphyrst 1>;
reset-names = "mac_core", "mac_ifc", "phy";
status = "disabled";
@@ -578,7 +583,9 @@
<&crg HISTB_PCIE_SYS_CLK>,
<&crg HISTB_PCIE_BUS_CLK>;
clock-names = "aux", "pipe", "sys", "bus";
- resets = <&crg 0x18c 6>, <&crg 0x18c 5>, <&crg 0x18c 4>;
+ resets = <&crg 0x18c 6 HISI_RESET_DEFAULT>,
+ <&crg 0x18c 5 HISI_RESET_DEFAULT>,
+ <&crg 0x18c 4 HISI_RESET_DEFAULT>;
reset-names = "soft", "sys", "bus";
phys = <&combphy1 PHY_TYPE_PCIE>;
phy-names = "phy";
@@ -593,7 +600,7 @@
<&crg HISTB_USB2_12M_CLK>,
<&crg HISTB_USB2_48M_CLK>;
clock-names = "bus", "clk12", "clk48";
- resets = <&crg 0xb8 12>;
+ resets = <&crg 0xb8 12 HISI_RESET_DEFAULT>;
reset-names = "bus";
phys = <&usb2_phy1_port0>;
phy-names = "usb";
@@ -608,9 +615,9 @@
<&crg HISTB_USB2_PHY_CLK>,
<&crg HISTB_USB2_UTMI_CLK>;
clock-names = "bus", "phy", "utmi";
- resets = <&crg 0xb8 12>,
- <&crg 0xb8 16>,
- <&crg 0xb8 13>;
+ resets = <&crg 0xb8 12 HISI_RESET_DEFAULT>,
+ <&crg 0xb8 16 HISI_RESET_DEFAULT>,
+ <&crg 0xb8 13 HISI_RESET_DEFAULT>;
reset-names = "bus", "phy", "utmi";
phys = <&usb2_phy1_port0>;
phy-names = "usb";
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] dt-bindings: clock: Update Hisilicon reset doc
2019-12-02 14:45 ` [PATCH 1/3] dt-bindings: clock: Update Hisilicon reset doc Jun Nie
@ 2019-12-02 17:04 ` Philipp Zabel
2019-12-03 3:11 ` Jun Nie
0 siblings, 1 reply; 11+ messages in thread
From: Philipp Zabel @ 2019-12-02 17:04 UTC (permalink / raw)
To: Jun Nie, mturquette, sboyd, robh+dt, mark.rutland, xuwei5,
opensource, swinslow, allison, yuehaibing, tglx, linux-clk,
devicetree, linux-arm-kernel, xuejiancheng
Hi Jun,
I have a few questions and comments about these patches. I notice that
the changed device trees only use the default setting. Are these new
features something that is required for the present SoCs, or is this in
preparation for a new SoC?
On Mon, 2019-12-02 at 22:45 +0800, Jun Nie wrote:
> Document the update of Hisilicon reset operation extension.
>
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
> .../devicetree/bindings/clock/hisi-crg.txt | 12 ++++----
> include/dt-bindings/reset/hisilicon-resets.h | 28 +++++++++++++++++++
> 2 files changed, 35 insertions(+), 5 deletions(-)
> create mode 100644 include/dt-bindings/reset/hisilicon-resets.h
>
> diff --git a/Documentation/devicetree/bindings/clock/hisi-crg.txt b/Documentation/devicetree/bindings/clock/hisi-crg.txt
> index cc60b3d423f3..fd8b0a964806 100644
> --- a/Documentation/devicetree/bindings/clock/hisi-crg.txt
> +++ b/Documentation/devicetree/bindings/clock/hisi-crg.txt
> @@ -26,19 +26,21 @@ to specify the clock which they consume.
>
> All these identifier could be found in <dt-bindings/clock/hi3519-clock.h>.
>
> -- #reset-cells: should be 2.
> +- #reset-cells: should be 3.
>
> A reset signal can be controlled by writing a bit register in the CRG module.
> -The reset specifier consists of two cells. The first cell represents the
> +The reset specifier consists of three cells. The first cell represents the
> register offset relative to the base address. The second cell represents the
> -bit index in the register.
> +bit index in the register. The third represent the flags to operation type.
> +
> +All reset flags could be found in <dt-bindings/reset/hisilicon-resets.h>
>
> Example: CRG nodes
> CRG: clock-reset-controller@12010000 {
> compatible = "hisilicon,hi3519-crg";
> reg = <0x12010000 0x10000>;
> #clock-cells = <1>;
> - #reset-cells = <2>;
> + #reset-cells = <3>;
> };
>
> Example: consumer nodes
> @@ -46,5 +48,5 @@ i2c0: i2c@12110000 {
> compatible = "hisilicon,hi3519-i2c";
> reg = <0x12110000 0x1000>;
> clocks = <&CRG HI3519_I2C0_RST>;
> - resets = <&CRG 0xe4 0>;
> + resets = <&CRG 0xe4 0 (HISI_ASSERT_SET | HISI_DEASSERT_CLEAR)>;
> };
> diff --git a/include/dt-bindings/reset/hisilicon-resets.h b/include/dt-bindings/reset/hisilicon-resets.h
> new file mode 100644
> index 000000000000..983e42a0c318
> --- /dev/null
> +++ b/include/dt-bindings/reset/hisilicon-resets.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Hisilicon Reset definitions
> + *
> + * Copyright (c) 2019 HiSilicon Technologies Co., Ltd.
> + */
> +
> +#ifndef __DT_BINDINGS_RESET_HISILICON_H__
> +#define __DT_BINDINGS_RESET_HISILICON_H__
> +
> +/*
> + * The reset does not support the feature and corresponding
> + * values are not valid
> + */
> +#define HISI_ASSERT_NONE (1 << 0)
> +#define HISI_DEASSERT_NONE (1 << 1)
What is the purpose of these two? Surely a reset control that does
nothing is not useful?
> +
> +/* When set this function is activated by polling/setting/clearing this bit */
> +#define HISI_ASSERT_SET (1 << 2)
> +#define HISI_DEASSERT_SET (1 << 3)
> +#define HISI_ASSERT_CLEAR (0 << 4)
> +#define HISI_DEASSERT_CLEAR (0 << 5)
> +#define HISI_ASSERT_POLL (0 << 6)
> +#define HISI_DEASSERT_POLL (0 << 7)
These are all zero, checking for them with an & operation in the code
always returns false.
> +
> +#define HISI_RESET_DEFAULT (HISI_ASSERT_SET | HISI_DEASSERT_CLEAR)
> +
> +#endif
regards
Philipp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] reset: hisilicon: Extend reset operation type
2019-12-02 14:45 ` [PATCH 2/3] reset: hisilicon: Extend reset operation type Jun Nie
@ 2019-12-02 17:04 ` Philipp Zabel
2019-12-03 3:53 ` Jun Nie
0 siblings, 1 reply; 11+ messages in thread
From: Philipp Zabel @ 2019-12-02 17:04 UTC (permalink / raw)
To: Jun Nie, mturquette, sboyd, robh+dt, mark.rutland, xuwei5,
opensource, swinslow, allison, yuehaibing, tglx, linux-clk,
devicetree, linux-arm-kernel, xuejiancheng
On Mon, 2019-12-02 at 22:45 +0800, Jun Nie wrote:
> Extend reset operations to support combination of three type flags:
> ASSERT/DEASSERT SET/CLEAR POLL.
>
> Signed-off-by: Jun Nie <jun.nie@linaro.org>
> ---
> drivers/clk/hisilicon/reset.c | 58 ++++++++++++++++++++++++++++++++---
> 1 file changed, 53 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
> index 93cee17db8b1..de7d186b0894 100644
> --- a/drivers/clk/hisilicon/reset.c
> +++ b/drivers/clk/hisilicon/reset.c
> @@ -2,20 +2,25 @@
> /*
> * Hisilicon Reset Controller Driver
> *
> - * Copyright (c) 2015-2016 HiSilicon Technologies Co., Ltd.
> + * Copyright (c) 2015-2019 HiSilicon Technologies Co., Ltd.
> */
>
> #include <linux/io.h>
> +#include <linux/iopoll.h>
> #include <linux/of_address.h>
> #include <linux/platform_device.h>
> #include <linux/reset-controller.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> +
> +#include <dt-bindings/reset/hisilicon-resets.h>
> #include "reset.h"
>
> #define HISI_RESET_BIT_MASK 0x1f
> #define HISI_RESET_OFFSET_SHIFT 8
> #define HISI_RESET_OFFSET_MASK 0xffff00
> +#define HISI_RESET_FLAG_SHIFT 24
> +#define HISI_RESET_FLAG_MASK 0xff000000
>
> struct hisi_reset_controller {
> spinlock_t lock;
> @@ -30,14 +35,17 @@ struct hisi_reset_controller {
> static int hisi_reset_of_xlate(struct reset_controller_dev *rcdev,
> const struct of_phandle_args *reset_spec)
> {
> + unsigned long flags;
> u32 offset;
> u8 bit;
>
> + flags = (reset_spec->args[2] << HISI_RESET_FLAG_SHIFT)
> + & HISI_RESET_FLAG_MASK;
> offset = (reset_spec->args[0] << HISI_RESET_OFFSET_SHIFT)
> & HISI_RESET_OFFSET_MASK;
> bit = reset_spec->args[1] & HISI_RESET_BIT_MASK;
>
> - return (offset | bit);
> + return (flags | offset | bit);
> }
>
> static int hisi_reset_assert(struct reset_controller_dev *rcdev,
> @@ -48,13 +56,33 @@ static int hisi_reset_assert(struct reset_controller_dev *rcdev,
> u32 offset, reg;
> u8 bit;
>
> + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> + if (flags & HISI_ASSERT_NONE)
> + return -ENOTSUPP; /* assert not supported for this reset */
As long as .assert and .deassert are the only implemented operations for
this reset controller, this does not make any sense to me. Are there
resets that can only be deasserted?
> +
> offset = (id & HISI_RESET_OFFSET_MASK) >> HISI_RESET_OFFSET_SHIFT;
> bit = id & HISI_RESET_BIT_MASK;
>
> + pr_devel("%s %s to %s 0x%x:bit[%d]\n", __func__,
> + flags & HISI_ASSERT_POLL ? "poll" : "",
> + flags & HISI_ASSERT_SET ? "set":"clear", offset, bit);
> +
> + if (flags & HISI_ASSERT_POLL) {
Since HISI_ASSERT_POLL is 0, this is always false.
> + if (flags & HISI_ASSERT_SET)
> + return readl_poll_timeout(rstc->membase + offset,
> + reg, reg & BIT(bit), 0, 5000);
> + else
> + return readl_poll_timeout(rstc->membase + offset,
> + reg, !(reg & BIT(bit)),
> + 0, 5000);
And this is effectively dead code. Do you really have hardware that
asserts or asserts a reset line in reaction to a read access?
Should HISI_ASSERT_POLL and HISI_DEASSERT_POLL be mutually exclusive?
> + }
> +
> spin_lock_irqsave(&rstc->lock, flags);
>
> reg = readl(rstc->membase + offset);
> - writel(reg | BIT(bit), rstc->membase + offset);
> + /* Default is setting to assert for no flag case. */
> + reg = (flags & HISI_ASSERT_CLEAR) ? reg & ~BIT(bit) : reg | BIT(bit);
Consider moving this into a helper function with a boolean set/clear
parameter.
As long as HISI_ASSERT_CLEAR is 0, the first branch is dead code.
> + writel(reg, rstc->membase + offset);
>
> spin_unlock_irqrestore(&rstc->lock, flags);
>
> @@ -69,13 +97,33 @@ static int hisi_reset_deassert(struct reset_controller_dev *rcdev,
> u32 offset, reg;
> u8 bit;
>
> + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> + if (flags & HISI_DEASSERT_NONE)
> + return -ENOTSUPP; /* deassert not supported for this reset */
> +
Are there resets that can only ever be asserted?
> offset = (id & HISI_RESET_OFFSET_MASK) >> HISI_RESET_OFFSET_SHIFT;
> bit = id & HISI_RESET_BIT_MASK;
>
> + pr_devel("%s %s to %s 0x%x:bit[%d]\n", __func__,
> + flags & HISI_DEASSERT_POLL ? "poll" : "",
> + flags & HISI_DEASSERT_SET ? "clear":"set", offset, bit);
> +
> + if (flags & HISI_DEASSERT_POLL) {
> + if (flags & HISI_DEASSERT_SET)
> + return readl_poll_timeout(rstc->membase + offset,
> + reg, reg & BIT(bit), 0, 5000);
> + else
> + return readl_poll_timeout(rstc->membase + offset,
> + reg, !(reg & BIT(bit)),
> + 0, 5000);
See above, this code currently can never be reached.
> + }
> +
> spin_lock_irqsave(&rstc->lock, flags);
>
> reg = readl(rstc->membase + offset);
> - writel(reg & ~BIT(bit), rstc->membase + offset);
> + /* Default is clearing to deasseart for no flag case. */
> + reg = (flags & HISI_DEASSERT_SET) ? reg | BIT(bit) : reg & ~BIT(bit);
Same as above, the read-modify-write for set/clear could be split out
into a helper.
> + writel(reg, rstc->membase + offset);
>
> spin_unlock_irqrestore(&rstc->lock, flags);
>
> @@ -103,7 +151,7 @@ struct hisi_reset_controller *hisi_reset_init(struct platform_device *pdev)
> rstc->rcdev.owner = THIS_MODULE;
> rstc->rcdev.ops = &hisi_reset_ops;
> rstc->rcdev.of_node = pdev->dev.of_node;
> - rstc->rcdev.of_reset_n_cells = 2;
> + rstc->rcdev.of_reset_n_cells = 3;
This breaks current device trees (before patch 3). You can make sure
device trees with #reset-cells = <2> keep working by parsing the #reset-
cells and setting this value to 2 for old DTs.
> rstc->rcdev.of_xlate = hisi_reset_of_xlate;
> reset_controller_register(&rstc->rcdev);
regards
Philipp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] dt-bindings: clock: Update Hisilicon reset doc
2019-12-02 17:04 ` Philipp Zabel
@ 2019-12-03 3:11 ` Jun Nie
2019-12-03 14:12 ` Philipp Zabel
0 siblings, 1 reply; 11+ messages in thread
From: Jun Nie @ 2019-12-03 3:11 UTC (permalink / raw)
To: Philipp Zabel
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Wei Xu,
opensource, swinslow, allison, yuehaibing, tglx, linux-clk,
devicetree, linux-arm-kernel, xuejiancheng
Philipp Zabel <p.zabel@pengutronix.de> 于2019年12月3日周二 上午1:04写道：
>
> Hi Jun,
>
> I have a few questions and comments about these patches. I notice that
> the changed device trees only use the default setting. Are these new
> features something that is required for the present SoCs, or is this in
> preparation for a new SoC?
Yes, this patch set is prepared for new SoC.
>
> On Mon, 2019-12-02 at 22:45 +0800, Jun Nie wrote:
> > Document the update of Hisilicon reset operation extension.
> >
> > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > ---
> > .../devicetree/bindings/clock/hisi-crg.txt | 12 ++++----
> > include/dt-bindings/reset/hisilicon-resets.h | 28 +++++++++++++++++++
> > 2 files changed, 35 insertions(+), 5 deletions(-)
> > create mode 100644 include/dt-bindings/reset/hisilicon-resets.h
> >
> > diff --git a/Documentation/devicetree/bindings/clock/hisi-crg.txt b/Documentation/devicetree/bindings/clock/hisi-crg.txt
> > index cc60b3d423f3..fd8b0a964806 100644
> > --- a/Documentation/devicetree/bindings/clock/hisi-crg.txt
> > +++ b/Documentation/devicetree/bindings/clock/hisi-crg.txt
> > @@ -26,19 +26,21 @@ to specify the clock which they consume.
> >
> > All these identifier could be found in <dt-bindings/clock/hi3519-clock.h>.
> >
> > -- #reset-cells: should be 2.
> > +- #reset-cells: should be 3.
> >
> > A reset signal can be controlled by writing a bit register in the CRG module.
> > -The reset specifier consists of two cells. The first cell represents the
> > +The reset specifier consists of three cells. The first cell represents the
> > register offset relative to the base address. The second cell represents the
> > -bit index in the register.
> > +bit index in the register. The third represent the flags to operation type.
> > +
> > +All reset flags could be found in <dt-bindings/reset/hisilicon-resets.h>
> >
> > Example: CRG nodes
> > CRG: clock-reset-controller@12010000 {
> > compatible = "hisilicon,hi3519-crg";
> > reg = <0x12010000 0x10000>;
> > #clock-cells = <1>;
> > - #reset-cells = <2>;
> > + #reset-cells = <3>;
> > };
> >
> > Example: consumer nodes
> > @@ -46,5 +48,5 @@ i2c0: i2c@12110000 {
> > compatible = "hisilicon,hi3519-i2c";
> > reg = <0x12110000 0x1000>;
> > clocks = <&CRG HI3519_I2C0_RST>;
> > - resets = <&CRG 0xe4 0>;
> > + resets = <&CRG 0xe4 0 (HISI_ASSERT_SET | HISI_DEASSERT_CLEAR)>;
> > };
> > diff --git a/include/dt-bindings/reset/hisilicon-resets.h b/include/dt-bindings/reset/hisilicon-resets.h
> > new file mode 100644
> > index 000000000000..983e42a0c318
> > --- /dev/null
> > +++ b/include/dt-bindings/reset/hisilicon-resets.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Hisilicon Reset definitions
> > + *
> > + * Copyright (c) 2019 HiSilicon Technologies Co., Ltd.
> > + */
> > +
> > +#ifndef __DT_BINDINGS_RESET_HISILICON_H__
> > +#define __DT_BINDINGS_RESET_HISILICON_H__
> > +
> > +/*
> > + * The reset does not support the feature and corresponding
> > + * values are not valid
> > + */
> > +#define HISI_ASSERT_NONE (1 << 0)
> > +#define HISI_DEASSERT_NONE (1 << 1)
>
> What is the purpose of these two? Surely a reset control that does
> nothing is not useful?
>
> > +
> > +/* When set this function is activated by polling/setting/clearing this bit */
> > +#define HISI_ASSERT_SET (1 << 2)
> > +#define HISI_DEASSERT_SET (1 << 3)
>
> > +#define HISI_ASSERT_CLEAR (0 << 4)
> > +#define HISI_DEASSERT_CLEAR (0 << 5)
> > +#define HISI_ASSERT_POLL (0 << 6)
> > +#define HISI_DEASSERT_POLL (0 << 7)
>
> These are all zero, checking for them with an & operation in the code
> always returns false.
Thanks for pointing out this! This is a typo in the early version
patch. I made some
mistake when preparing the patch for upstream. Will fix this issue.
>
> > +
> > +#define HISI_RESET_DEFAULT (HISI_ASSERT_SET | HISI_DEASSERT_CLEAR)
> > +
> > +#endif
>
> regards
> Philipp
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] reset: hisilicon: Extend reset operation type
2019-12-02 17:04 ` Philipp Zabel
@ 2019-12-03 3:53 ` Jun Nie
2019-12-03 14:15 ` Philipp Zabel
0 siblings, 1 reply; 11+ messages in thread
From: Jun Nie @ 2019-12-03 3:53 UTC (permalink / raw)
To: Philipp Zabel
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Wei Xu,
opensource, swinslow, allison, yuehaibing, tglx, linux-clk,
devicetree, linux-arm-kernel, xuejiancheng
Philipp Zabel <p.zabel@pengutronix.de> 于2019年12月3日周二 上午1:04写道：
>
> On Mon, 2019-12-02 at 22:45 +0800, Jun Nie wrote:
> > Extend reset operations to support combination of three type flags:
> > ASSERT/DEASSERT SET/CLEAR POLL.
> >
> > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > ---
> > drivers/clk/hisilicon/reset.c | 58 ++++++++++++++++++++++++++++++++---
> > 1 file changed, 53 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
> > index 93cee17db8b1..de7d186b0894 100644
> > --- a/drivers/clk/hisilicon/reset.c
> > +++ b/drivers/clk/hisilicon/reset.c
> > @@ -2,20 +2,25 @@
> > /*
> > * Hisilicon Reset Controller Driver
> > *
> > - * Copyright (c) 2015-2016 HiSilicon Technologies Co., Ltd.
> > + * Copyright (c) 2015-2019 HiSilicon Technologies Co., Ltd.
> > */
> >
> > #include <linux/io.h>
> > +#include <linux/iopoll.h>
> > #include <linux/of_address.h>
> > #include <linux/platform_device.h>
> > #include <linux/reset-controller.h>
> > #include <linux/slab.h>
> > #include <linux/spinlock.h>
> > +
> > +#include <dt-bindings/reset/hisilicon-resets.h>
> > #include "reset.h"
> >
> > #define HISI_RESET_BIT_MASK 0x1f
> > #define HISI_RESET_OFFSET_SHIFT 8
> > #define HISI_RESET_OFFSET_MASK 0xffff00
> > +#define HISI_RESET_FLAG_SHIFT 24
> > +#define HISI_RESET_FLAG_MASK 0xff000000
> >
> > struct hisi_reset_controller {
> > spinlock_t lock;
> > @@ -30,14 +35,17 @@ struct hisi_reset_controller {
> > static int hisi_reset_of_xlate(struct reset_controller_dev *rcdev,
> > const struct of_phandle_args *reset_spec)
> > {
> > + unsigned long flags;
> > u32 offset;
> > u8 bit;
> >
> > + flags = (reset_spec->args[2] << HISI_RESET_FLAG_SHIFT)
> > + & HISI_RESET_FLAG_MASK;
> > offset = (reset_spec->args[0] << HISI_RESET_OFFSET_SHIFT)
> > & HISI_RESET_OFFSET_MASK;
> > bit = reset_spec->args[1] & HISI_RESET_BIT_MASK;
> >
> > - return (offset | bit);
> > + return (flags | offset | bit);
> > }
> >
> > static int hisi_reset_assert(struct reset_controller_dev *rcdev,
> > @@ -48,13 +56,33 @@ static int hisi_reset_assert(struct reset_controller_dev *rcdev,
> > u32 offset, reg;
> > u8 bit;
> >
> > + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> > + if (flags & HISI_ASSERT_NONE)
> > + return -ENOTSUPP; /* assert not supported for this reset */
>
> As long as .assert and .deassert are the only implemented operations for
> this reset controller, this does not make any sense to me. Are there
> resets that can only be deasserted?
Some reset is asserted on power on automatically. So only .deassert is needed.
>
> > +
> > offset = (id & HISI_RESET_OFFSET_MASK) >> HISI_RESET_OFFSET_SHIFT;
> > bit = id & HISI_RESET_BIT_MASK;
> >
> > + pr_devel("%s %s to %s 0x%x:bit[%d]\n", __func__,
> > + flags & HISI_ASSERT_POLL ? "poll" : "",
> > + flags & HISI_ASSERT_SET ? "set":"clear", offset, bit);
> > +
> > + if (flags & HISI_ASSERT_POLL) {
>
> Since HISI_ASSERT_POLL is 0, this is always false.
Will fix the wrong value definition in next version patch. The same to
below comments.
>
> > + if (flags & HISI_ASSERT_SET)
> > + return readl_poll_timeout(rstc->membase + offset,
> > + reg, reg & BIT(bit), 0, 5000);
> > + else
> > + return readl_poll_timeout(rstc->membase + offset,
> > + reg, !(reg & BIT(bit)),
> > + 0, 5000);
>
> And this is effectively dead code. Do you really have hardware that
> asserts or asserts a reset line in reaction to a read access?
>
> Should HISI_ASSERT_POLL and HISI_DEASSERT_POLL be mutually exclusive?
>
> > + }
> > +
> > spin_lock_irqsave(&rstc->lock, flags);
> >
> > reg = readl(rstc->membase + offset);
> > - writel(reg | BIT(bit), rstc->membase + offset);
> > + /* Default is setting to assert for no flag case. */
> > + reg = (flags & HISI_ASSERT_CLEAR) ? reg & ~BIT(bit) : reg | BIT(bit);
>
> Consider moving this into a helper function with a boolean set/clear
> parameter.
Will do.
>
> As long as HISI_ASSERT_CLEAR is 0, the first branch is dead code.
>
> > + writel(reg, rstc->membase + offset);
> >
> > spin_unlock_irqrestore(&rstc->lock, flags);
> >
> > @@ -69,13 +97,33 @@ static int hisi_reset_deassert(struct reset_controller_dev *rcdev,
> > u32 offset, reg;
> > u8 bit;
> >
> > + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> > + if (flags & HISI_DEASSERT_NONE)
> > + return -ENOTSUPP; /* deassert not supported for this reset */
> > +
>
> Are there resets that can only ever be asserted?
I do not know yet. Only extend this driver with all combination in logic.
>
> > offset = (id & HISI_RESET_OFFSET_MASK) >> HISI_RESET_OFFSET_SHIFT;
> > bit = id & HISI_RESET_BIT_MASK;
> >
> > + pr_devel("%s %s to %s 0x%x:bit[%d]\n", __func__,
> > + flags & HISI_DEASSERT_POLL ? "poll" : "",
> > + flags & HISI_DEASSERT_SET ? "clear":"set", offset, bit);
> > +
> > + if (flags & HISI_DEASSERT_POLL) {
> > + if (flags & HISI_DEASSERT_SET)
> > + return readl_poll_timeout(rstc->membase + offset,
> > + reg, reg & BIT(bit), 0, 5000);
> > + else
> > + return readl_poll_timeout(rstc->membase + offset,
> > + reg, !(reg & BIT(bit)),
> > + 0, 5000);
>
> See above, this code currently can never be reached.
>
> > + }
> > +
> > spin_lock_irqsave(&rstc->lock, flags);
> >
> > reg = readl(rstc->membase + offset);
> > - writel(reg & ~BIT(bit), rstc->membase + offset);
> > + /* Default is clearing to deasseart for no flag case. */
> > + reg = (flags & HISI_DEASSERT_SET) ? reg | BIT(bit) : reg & ~BIT(bit);
>
> Same as above, the read-modify-write for set/clear could be split out
> into a helper.
>
> > + writel(reg, rstc->membase + offset);
> >
> > spin_unlock_irqrestore(&rstc->lock, flags);
> >
> > @@ -103,7 +151,7 @@ struct hisi_reset_controller *hisi_reset_init(struct platform_device *pdev)
> > rstc->rcdev.owner = THIS_MODULE;
> > rstc->rcdev.ops = &hisi_reset_ops;
> > rstc->rcdev.of_node = pdev->dev.of_node;
> > - rstc->rcdev.of_reset_n_cells = 2;
> > + rstc->rcdev.of_reset_n_cells = 3;
>
> This breaks current device trees (before patch 3). You can make sure
> device trees with #reset-cells = <2> keep working by parsing the #reset-
> cells and setting this value to 2 for old DTs.
All current dts file are modified accordingly. But existing dtb binary support
is an issue. Do you have any suggestion?
>
> > rstc->rcdev.of_xlate = hisi_reset_of_xlate;
> > reset_controller_register(&rstc->rcdev);
>
> regards
> Philipp
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] dt-bindings: clock: Update Hisilicon reset doc
2019-12-03 3:11 ` Jun Nie
@ 2019-12-03 14:12 ` Philipp Zabel
0 siblings, 0 replies; 11+ messages in thread
From: Philipp Zabel @ 2019-12-03 14:12 UTC (permalink / raw)
To: Jun Nie
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Wei Xu,
opensource, swinslow, allison, yuehaibing, tglx, linux-clk,
devicetree, linux-arm-kernel, xuejiancheng
On Tue, 2019-12-03 at 11:11 +0800, Jun Nie wrote:
> Philipp Zabel <p.zabel@pengutronix.de> 于2019年12月3日周二 上午1:04写道：
> > Hi Jun,
> >
> > I have a few questions and comments about these patches. I notice that
> > the changed device trees only use the default setting. Are these new
> > features something that is required for the present SoCs, or is this in
> > preparation for a new SoC?
>
> Yes, this patch set is prepared for new SoC.
>
> > On Mon, 2019-12-02 at 22:45 +0800, Jun Nie wrote:
> > > Document the update of Hisilicon reset operation extension.
> > >
> > > Signed-off-by: Jun Nie <jun.nie@linaro.org>
> > > ---
> > > .../devicetree/bindings/clock/hisi-crg.txt | 12 ++++----
> > > include/dt-bindings/reset/hisilicon-resets.h | 28 +++++++++++++++++++
> > > 2 files changed, 35 insertions(+), 5 deletions(-)
> > > create mode 100644 include/dt-bindings/reset/hisilicon-resets.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/clock/hisi-crg.txt b/Documentation/devicetree/bindings/clock/hisi-crg.txt
> > > index cc60b3d423f3..fd8b0a964806 100644
> > > --- a/Documentation/devicetree/bindings/clock/hisi-crg.txt
> > > +++ b/Documentation/devicetree/bindings/clock/hisi-crg.txt
> > > @@ -26,19 +26,21 @@ to specify the clock which they consume.
> > >
> > > All these identifier could be found in <dt-bindings/clock/hi3519-clock.h>.
> > >
> > > -- #reset-cells: should be 2.
> > > +- #reset-cells: should be 3.
If this is only needed for a new SoC, you should introduce a new
compatible and leave #reset-cells = <2> for the old compatible. The new
compatible can require #reset-cells = <3>. Wit this, the current device
trees don't have to be changed at all.
regards
Philipp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] reset: hisilicon: Extend reset operation type
2019-12-03 3:53 ` Jun Nie
@ 2019-12-03 14:15 ` Philipp Zabel
2019-12-04 6:09 ` Jun Nie
0 siblings, 1 reply; 11+ messages in thread
From: Philipp Zabel @ 2019-12-03 14:15 UTC (permalink / raw)
To: Jun Nie
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Wei Xu,
opensource, swinslow, allison, yuehaibing, tglx, linux-clk,
devicetree, linux-arm-kernel, xuejiancheng
Hi Jun,
On Tue, 2019-12-03 at 11:53 +0800, Jun Nie wrote:
[...]
> @@ -48,13 +56,33 @@ static int hisi_reset_assert(struct reset_controller_dev *rcdev,
> > > u32 offset, reg;
> > > u8 bit;
> > >
> > > + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> > > + if (flags & HISI_ASSERT_NONE)
> > > + return -ENOTSUPP; /* assert not supported for this reset */
> >
> > As long as .assert and .deassert are the only implemented operations for
> > this reset controller, this does not make any sense to me. Are there
> > resets that can only be deasserted?
>
> Some reset is asserted on power on automatically. So only .deassert is needed.
But if the bit was set/cleared again after being deasserted, would it
assert the reset line? Basically, I wonder if those bits are write-once
or not.
[...]
> > > + if (flags & HISI_ASSERT_SET)
> > > + return readl_poll_timeout(rstc->membase + offset,
> > > + reg, reg & BIT(bit), 0, 5000);
> > > + else
> > > + return readl_poll_timeout(rstc->membase + offset,
> > > + reg, !(reg & BIT(bit)),
> > > + 0, 5000);
> >
> > And this is effectively dead code. Do you really have hardware that
> > asserts or asserts a reset line in reaction to a read access?
> >
> > Should HISI_ASSERT_POLL and HISI_DEASSERT_POLL be mutually exclusive?
This is missing an explanation.
[...]
> > > + writel(reg, rstc->membase + offset);
> > >
> > > spin_unlock_irqrestore(&rstc->lock, flags);
> > >
> > > @@ -69,13 +97,33 @@ static int hisi_reset_deassert(struct reset_controller_dev *rcdev,
> > > u32 offset, reg;
> > > u8 bit;
> > >
> > > + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> > > + if (flags & HISI_DEASSERT_NONE)
> > > + return -ENOTSUPP; /* deassert not supported for this reset */
> > > +
> >
> > Are there resets that can only ever be asserted?
>
> I do not know yet. Only extend this driver with all combination in logic.
If this is not used, let's leave it out.
[...]
> > > @@ -103,7 +151,7 @@ struct hisi_reset_controller *hisi_reset_init(struct platform_device *pdev)
> > > rstc->rcdev.owner = THIS_MODULE;
> > > rstc->rcdev.ops = &hisi_reset_ops;
> > > rstc->rcdev.of_node = pdev->dev.of_node;
> > > - rstc->rcdev.of_reset_n_cells = 2;
> > > + rstc->rcdev.of_reset_n_cells = 3;
> >
> > This breaks current device trees (before patch 3). You can make sure
> > device trees with #reset-cells = <2> keep working by parsing the #reset-
> > cells and setting this value to 2 for old DTs.
>
> All current dts file are modified accordingly. But existing dtb binary support
> is an issue. Do you have any suggestion?
Since this is for a new SoC, you should keep using of_reset_n_cells = 2
for the current SoCs and only set it to 3 for a new compatible, for
example using of_device_get_match_data().
regards
Philipp
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] reset: hisilicon: Extend reset operation type
2019-12-03 14:15 ` Philipp Zabel
@ 2019-12-04 6:09 ` Jun Nie
0 siblings, 0 replies; 11+ messages in thread
From: Jun Nie @ 2019-12-04 6:09 UTC (permalink / raw)
To: Philipp Zabel
Cc: Michael Turquette, sboyd, Rob Herring, Mark Rutland, Wei Xu,
opensource, swinslow, allison, yuehaibing, tglx, linux-clk,
devicetree, linux-arm-kernel, xuejiancheng
Philipp Zabel <p.zabel@pengutronix.de> 于2019年12月3日周二 下午10:15写道：
>
> Hi Jun,
>
> On Tue, 2019-12-03 at 11:53 +0800, Jun Nie wrote:
> [...]
> > @@ -48,13 +56,33 @@ static int hisi_reset_assert(struct reset_controller_dev *rcdev,
> > > > u32 offset, reg;
> > > > u8 bit;
> > > >
> > > > + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> > > > + if (flags & HISI_ASSERT_NONE)
> > > > + return -ENOTSUPP; /* assert not supported for this reset */
> > >
> > > As long as .assert and .deassert are the only implemented operations for
> > > this reset controller, this does not make any sense to me. Are there
> > > resets that can only be deasserted?
> >
> > Some reset is asserted on power on automatically. So only .deassert is needed.
>
> But if the bit was set/cleared again after being deasserted, would it
> assert the reset line? Basically, I wonder if those bits are write-once
> or not.
I did not see vendor's code to clear it, seems the bit is only polled
to be set on every
device enablement.
>
> [...]
> > > > + if (flags & HISI_ASSERT_SET)
> > > > + return readl_poll_timeout(rstc->membase + offset,
> > > > + reg, reg & BIT(bit), 0, 5000);
> > > > + else
> > > > + return readl_poll_timeout(rstc->membase + offset,
> > > > + reg, !(reg & BIT(bit)),
> > > > + 0, 5000);
> > >
> > > And this is effectively dead code. Do you really have hardware that
> > > asserts or asserts a reset line in reaction to a read access?
> > >
> > > Should HISI_ASSERT_POLL and HISI_DEASSERT_POLL be mutually exclusive?
>
> This is missing an explanation.
They are not mutually exclusive in logic. But I did not see the need
to poll for a assert yet.
>
> [...]
> > > > + writel(reg, rstc->membase + offset);
> > > >
> > > > spin_unlock_irqrestore(&rstc->lock, flags);
> > > >
> > > > @@ -69,13 +97,33 @@ static int hisi_reset_deassert(struct reset_controller_dev *rcdev,
> > > > u32 offset, reg;
> > > > u8 bit;
> > > >
> > > > + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> > > > + if (flags & HISI_DEASSERT_NONE)
> > > > + return -ENOTSUPP; /* deassert not supported for this reset */
> > > > +
> > >
> > > Are there resets that can only ever be asserted?
> >
> > I do not know yet. Only extend this driver with all combination in logic.
>
> If this is not used, let's leave it out.
>
> [...]
> > > > @@ -103,7 +151,7 @@ struct hisi_reset_controller *hisi_reset_init(struct platform_device *pdev)
> > > > rstc->rcdev.owner = THIS_MODULE;
> > > > rstc->rcdev.ops = &hisi_reset_ops;
> > > > rstc->rcdev.of_node = pdev->dev.of_node;
> > > > - rstc->rcdev.of_reset_n_cells = 2;
> > > > + rstc->rcdev.of_reset_n_cells = 3;
> > >
> > > This breaks current device trees (before patch 3). You can make sure
> > > device trees with #reset-cells = <2> keep working by parsing the #reset-
> > > cells and setting this value to 2 for old DTs.
> >
> > All current dts file are modified accordingly. But existing dtb binary support
> > is an issue. Do you have any suggestion?
>
> Since this is for a new SoC, you should keep using of_reset_n_cells = 2
> for the current SoCs and only set it to 3 for a new compatible, for
> example using of_device_get_match_data().
Yeah, this should be the solution. I will check the detail, thanks!
Regards,
Jun
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-12-04 7:06 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02 14:45 [PATCH 0/3] Extend Hisilicon reset type Jun Nie
2019-12-02 14:45 ` [PATCH 1/3] dt-bindings: clock: Update Hisilicon reset doc Jun Nie
2019-12-02 17:04 ` Philipp Zabel
2019-12-03 3:11 ` Jun Nie
2019-12-03 14:12 ` Philipp Zabel
2019-12-02 14:45 ` [PATCH 2/3] reset: hisilicon: Extend reset operation type Jun Nie
2019-12-02 17:04 ` Philipp Zabel
2019-12-03 3:53 ` Jun Nie
2019-12-03 14:15 ` Philipp Zabel
2019-12-04 6:09 ` Jun Nie
2019-12-02 14:45 ` [PATCH 3/3] ARM: dts: Update reset for hi3519 and hi3798cv200 Jun Nie
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).