| Message ID | 20191202144524.5391-2-jun.nie@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Extend Hisilicon reset type | expand |
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
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 >
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
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
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