[PATCH 0/3] Add reset controllers for STiH407 SoC

[PATCH 0/3] Add reset controllers for STiH407 SoC

[Peter Griffin]

This series adds reset controller support for the STiH407 SoC.

Peter Griffin (3):
  drivers: reset: stih407: Add softreset, powerdown and picophy
    controllers
  ARM: sti: Add STiH407 Kconfig entry to select STIH407_RESET
  ARM: STi: STiH407: Add reset controller support.

 .../bindings/reset/st,sti-picophyreset.txt         |  41 ++++++
 arch/arm/boot/dts/stih407.dtsi                     |  16 +++
 arch/arm/mach-sti/Kconfig                          |  10 ++
 drivers/reset/sti/Kconfig                          |   4 +
 drivers/reset/sti/Makefile                         |   1 +
 drivers/reset/sti/reset-stih407.c                  | 159 +++++++++++++++++++++
 .../dt-bindings/reset-controller/stih407-resets.h  |  60 ++++++++
 7 files changed, 291 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt
 create mode 100644 drivers/reset/sti/reset-stih407.c
 create mode 100644 include/dt-bindings/reset-controller/stih407-resets.h

-- 
1.9.1

[PATCH 1/3] drivers: reset: stih407: Add softreset, powerdown and picophy controllers

[Peter Griffin]

This patch adds a softreset, powerdown and picophy reset controllers for
the STiH407 SoC.

With this patch three new devices are registered: -
1. st,stih407-powerdown
2. st,stih407-softreset
3. st,stih407-picophyreset

All three devices use system configuration registers mapped via regmap to
perform the reset or powerdown. The powerdown controller also has
an acknowledgement.

A separate picophy reset controller manages the different reset channels within
the picophy, which have a different polarity to the other system softresets.
Managing these different picophy softreset channels is necessary to correctly
handle resuming from CPS when USB2 devices are plugged into the USB3 port.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro-qxv4g6HH51o@public.gmane.org>
Signed-off-by: Peter Griffin <peter.griffin-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 .../bindings/reset/st,sti-picophyreset.txt         |  41 ++++++
 drivers/reset/sti/Kconfig                          |   4 +
 drivers/reset/sti/Makefile                         |   1 +
 drivers/reset/sti/reset-stih407.c                  | 159 +++++++++++++++++++++
 .../dt-bindings/reset-controller/stih407-resets.h  |  60 ++++++++
 5 files changed, 265 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt
 create mode 100644 drivers/reset/sti/reset-stih407.c
 create mode 100644 include/dt-bindings/reset-controller/stih407-resets.h

diff --git a/Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt b/Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt
new file mode 100644
index 0000000..4c756d1
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt
@@ -0,0 +1,41 @@
+STMicroelectronics STi family Sysconfig Picophy SoftReset Controller
+=============================================================================
+
+This binding describes a reset controller device that is used to enable and
+disable on-chip PicoPHY USB2 phy(s) using "softreset" control bits found in
+the STi family SoC system configuration registers.
+
+The actual action taken when softreset is asserted is hardware dependent.
+However, when asserted it may not be possible to access the hardware's
+registers and after an assert/deassert sequence the hardware's previous state
+may no longer be valid.
+
+Please refer to Documentation/devicetree/bindings/reset/reset.txt
+for common reset controller binding usage.
+
+Required properties:
+- compatible: Should be "st,<chip>-softreset"
+- #reset-cells: 1, see below
+
+example:
+
+	picophyreset: picophyreset-controller {
+		#reset-cells = <1>;
+		compatible = "st,stih407-picophyreset";
+	};
+
+Specifying picophyreset control of devices
+=======================================
+
+Device nodes should specify the reset channel required in their "resets"
+property, containing a phandle to the picophyreset device node and an
+index specifying which channel to use, as described in
+Documentation/devicetree/bindings/reset/reset.txt.
+
+example:
+	usb2_picophy0: usbpicophy@0 {
+		resets = <&picophyreset STIH407_PICOPHY0_RESET>;
+	};
+
+Macro definitions for the supported reset channels can be found in:
+include/dt-bindings/reset-controller/stih407-resets.h
diff --git a/drivers/reset/sti/Kconfig b/drivers/reset/sti/Kconfig
index 88d2d03..f8c15a3 100644
--- a/drivers/reset/sti/Kconfig
+++ b/drivers/reset/sti/Kconfig
@@ -12,4 +12,8 @@ config STIH416_RESET
 	bool
 	select STI_RESET_SYSCFG
 
+config STIH407_RESET
+	bool
+	select STI_RESET_SYSCFG
+
 endif
diff --git a/drivers/reset/sti/Makefile b/drivers/reset/sti/Makefile
index be1c976..dc85dfb 100644
--- a/drivers/reset/sti/Makefile
+++ b/drivers/reset/sti/Makefile
@@ -2,3 +2,4 @@ obj-$(CONFIG_STI_RESET_SYSCFG) += reset-syscfg.o
 
 obj-$(CONFIG_STIH415_RESET) += reset-stih415.o
 obj-$(CONFIG_STIH416_RESET) += reset-stih416.o
+obj-$(CONFIG_STIH407_RESET) += reset-stih407.o
diff --git a/drivers/reset/sti/reset-stih407.c b/drivers/reset/sti/reset-stih407.c
new file mode 100644
index 0000000..1650792
--- /dev/null
+++ b/drivers/reset/sti/reset-stih407.c
@@ -0,0 +1,159 @@
+/*
+ * Copyright (C) 2013 STMicroelectronics (R&D) Limited
+ * Author: Giuseppe Cavallaro <peppe.cavallaro-qxv4g6HH51o@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#include <dt-bindings/reset-controller/stih407-resets.h>
+
+#include "reset-syscfg.h"
+
+/*
+ * STiH407 Peripheral powerdown definitions.
+ */
+static const char stih407_core[] = "st,stih407-core-syscfg";
+static const char stih407_sbc_reg[] = "st,stih407-sbc-reg-syscfg";
+static const char stih407_lpm[] = "st,stih407-lpm-syscfg";
+
+#define STIH407_PDN_0(_bit) \
+	_SYSCFG_RST_CH(stih407_core, SYSCFG_5000, _bit, SYSSTAT_5500, _bit)
+#define STIH407_PDN_1(_bit) \
+	_SYSCFG_RST_CH(stih407_core, SYSCFG_5001, _bit, SYSSTAT_5501, _bit)
+#define STIH407_PDN_ETH(_bit, _stat) \
+	_SYSCFG_RST_CH(stih407_sbc_reg, SYSCFG_4032, _bit, SYSSTAT_4520, _stat)
+
+/* Powerdown requests control 0 */
+#define SYSCFG_5000	0x0
+#define SYSSTAT_5500	0x7d0
+/* Powerdown requests control 1 (High Speed Links) */
+#define SYSCFG_5001	0x4
+#define SYSSTAT_5501	0x7d4
+
+/* Ethernet powerdown/status/reset */
+#define	SYSCFG_4032	0x80
+#define	SYSSTAT_4520	0x820
+
+#define	SYSCFG_4002	0x8
+
+static const struct syscfg_reset_channel_data stih407_powerdowns[] = {
+	[STIH407_EMISS_POWERDOWN] = STIH407_PDN_0(1),
+	[STIH407_NAND_POWERDOWN] = STIH407_PDN_0(0),
+	[STIH407_USB3_POWERDOWN] = STIH407_PDN_1(6),
+	[STIH407_USB2_PORT1_POWERDOWN] = STIH407_PDN_1(5),
+	[STIH407_USB2_PORT0_POWERDOWN] = STIH407_PDN_1(4),
+	[STIH407_PCIE1_POWERDOWN] = STIH407_PDN_1(3),
+	[STIH407_PCIE0_POWERDOWN] = STIH407_PDN_1(2),
+	[STIH407_SATA1_POWERDOWN] = STIH407_PDN_1(1),
+	[STIH407_SATA0_POWERDOWN] = STIH407_PDN_1(0),
+	[STIH407_ETH1_POWERDOWN] = STIH407_PDN_ETH(0, 2),
+};
+
+/* Reset Generator control 0/1 */
+#define	SYSCFG_5131	0x20c
+#define	SYSCFG_5132	0x210
+
+#define LPM_SYSCFG_1    0x4	/* Softreset IRB & SBC UART */
+
+#define STIH407_SRST_CORE(_reg, _bit) \
+	_SYSCFG_RST_CH_NO_ACK(stih407_core, _reg, _bit)
+
+#define STIH407_SRST_SBC(_reg, _bit) \
+	_SYSCFG_RST_CH_NO_ACK(stih407_sbc_reg, _reg, _bit)
+
+#define STIH407_SRST_LPM(_reg, _bit) \
+	_SYSCFG_RST_CH_NO_ACK(stih407_lpm, _reg, _bit)
+
+static const struct syscfg_reset_channel_data stih407_softresets[] = {
+	[STIH407_ETH1_SOFTRESET] = STIH407_SRST_SBC(SYSCFG_4002, 4),
+	[STIH407_MMC1_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 3),
+	[STIH407_USB2_PORT0_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 28),
+	[STIH407_USB2_PORT1_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 29),
+	[STIH407_PICOPHY_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 30),
+	[STIH407_IRB_SOFTRESET] = STIH407_SRST_LPM(LPM_SYSCFG_1, 6),
+	[STIH407_PCIE0_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 6),
+	[STIH407_PCIE1_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 15),
+	[STIH407_SATA0_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 7),
+	[STIH407_SATA1_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 16),
+	[STIH407_MIPHY0_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 4),
+	[STIH407_MIPHY1_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 13),
+	[STIH407_MIPHY2_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 22),
+	[STIH407_SATA0_PWR_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 5),
+	[STIH407_SATA1_PWR_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 14),
+	[STIH407_DELTA_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 3),
+	[STIH407_BLITTER_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 10),
+	[STIH407_HDTVOUT_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 11),
+	[STIH407_HDQVDP_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 12),
+	[STIH407_VDP_AUX_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 14),
+	[STIH407_COMPO_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 15),
+	[STIH407_HDMI_TX_PHY_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 21),
+	[STIH407_JPEG_DEC_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 23),
+	[STIH407_VP8_DEC_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 24),
+	[STIH407_GPU_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 30),
+	[STIH407_HVA_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 0),
+	[STIH407_ERAM_HVA_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 1),
+	[STIH407_LPM_SOFTRESET] = STIH407_SRST_SBC(SYSCFG_4002, 2),
+	[STIH407_KEYSCAN_SOFTRESET] = STIH407_SRST_LPM(LPM_SYSCFG_1, 8),
+};
+
+/* PicoPHY reset/control */
+#define SYSCFG_5061	0x0f4
+
+static const struct syscfg_reset_channel_data stih407_picophyresets[] = {
+	[STIH407_PICOPHY0_RESET] = STIH407_SRST_CORE(SYSCFG_5061, 5),
+	[STIH407_PICOPHY1_RESET] = STIH407_SRST_CORE(SYSCFG_5061, 6),
+	[STIH407_PICOPHY2_RESET] = STIH407_SRST_CORE(SYSCFG_5061, 7),
+};
+
+static struct syscfg_reset_controller_data stih407_powerdown_controller = {
+	.wait_for_ack = true,
+	.nr_channels = ARRAY_SIZE(stih407_powerdowns),
+	.channels = stih407_powerdowns,
+};
+
+static struct syscfg_reset_controller_data stih407_softreset_controller = {
+	.wait_for_ack = false,
+	.active_low = true,
+	.nr_channels = ARRAY_SIZE(stih407_softresets),
+	.channels = stih407_softresets,
+};
+
+static struct syscfg_reset_controller_data stih407_picophyreset_controller = {
+	.wait_for_ack = false,
+	.nr_channels = ARRAY_SIZE(stih407_picophyresets),
+	.channels = stih407_picophyresets,
+};
+
+
+static struct of_device_id stih407_reset_match[] = {
+	{.compatible = "st,stih407-powerdown",
+	 .data = &stih407_powerdown_controller,},
+	{.compatible = "st,stih407-softreset",
+	 .data = &stih407_softreset_controller,},
+	{.compatible = "st,stih407-picophyreset",
+	 .data = &stih407_picophyreset_controller,},
+	{},
+};
+
+static struct platform_driver stih407_reset_driver = {
+	.probe = syscfg_reset_probe,
+	.driver = {
+		   .name = "reset-stih407",
+		   .owner = THIS_MODULE,
+		   .of_match_table = stih407_reset_match,
+		   },
+};
+
+static int __init stih407_reset_init(void)
+{
+	return platform_driver_register(&stih407_reset_driver);
+}
+
+arch_initcall(stih407_reset_init);
diff --git a/include/dt-bindings/reset-controller/stih407-resets.h b/include/dt-bindings/reset-controller/stih407-resets.h
new file mode 100644
index 0000000..85448a3
--- /dev/null
+++ b/include/dt-bindings/reset-controller/stih407-resets.h
@@ -0,0 +1,60 @@
+/*
+ * This header provides constants for the reset controller
+ * based peripheral powerdown requests on the STMicroelectronics
+ * STiH407 SoC.
+ */
+#ifndef _DT_BINDINGS_RESET_CONTROLLER_STIH407
+#define _DT_BINDINGS_RESET_CONTROLLER_STIH407
+
+/* Powerdown requests control 0 */
+#define STIH407_EMISS_POWERDOWN		0
+#define STIH407_NAND_POWERDOWN		1
+
+/* Synp GMAC PowerDown */
+#define	STIH407_ETH1_POWERDOWN		2
+/* Powerdown requests control 1 */
+#define STIH407_USB3_POWERDOWN		3
+#define STIH407_USB2_PORT1_POWERDOWN	4
+#define STIH407_USB2_PORT0_POWERDOWN	5
+#define STIH407_PCIE1_POWERDOWN		6
+#define STIH407_PCIE0_POWERDOWN		7
+#define STIH407_SATA1_POWERDOWN		8
+#define STIH407_SATA0_POWERDOWN		9
+
+/* Reset defines */
+#define	STIH407_ETH1_SOFTRESET		0
+#define	STIH407_MMC1_SOFTRESET		1
+#define	STIH407_PICOPHY_SOFTRESET	2
+#define	STIH407_IRB_SOFTRESET		3
+#define	STIH407_PCIE0_SOFTRESET		4
+#define	STIH407_PCIE1_SOFTRESET		5
+#define	STIH407_SATA0_SOFTRESET		6
+#define	STIH407_SATA1_SOFTRESET		7
+#define	STIH407_MIPHY0_SOFTRESET	8
+#define	STIH407_MIPHY1_SOFTRESET	9
+#define	STIH407_MIPHY2_SOFTRESET	10
+#define	STIH407_SATA0_PWR_SOFTRESET	11
+#define	STIH407_SATA1_PWR_SOFTRESET	12
+#define	STIH407_DELTA_SOFTRESET		13
+#define	STIH407_BLITTER_SOFTRESET	14
+#define	STIH407_HDTVOUT_SOFTRESET	15
+#define	STIH407_HDQVDP_SOFTRESET	16
+#define	STIH407_VDP_AUX_SOFTRESET	17
+#define	STIH407_COMPO_SOFTRESET		18
+#define	STIH407_HDMI_TX_PHY_SOFTRESET	19
+#define	STIH407_JPEG_DEC_SOFTRESET	20
+#define	STIH407_VP8_DEC_SOFTRESET	21
+#define	STIH407_GPU_SOFTRESET		22
+#define	STIH407_HVA_SOFTRESET		23
+#define	STIH407_ERAM_HVA_SOFTRESET	24
+#define	STIH407_LPM_SOFTRESET		25
+#define	STIH407_KEYSCAN_SOFTRESET	26
+#define	STIH407_USB2_PORT0_SOFTRESET	27
+#define	STIH407_USB2_PORT1_SOFTRESET	28
+
+/* Picophy reset defines */
+#define	STIH407_PICOPHY0_RESET	0
+#define	STIH407_PICOPHY1_RESET	1
+#define	STIH407_PICOPHY2_RESET	2
+
+#endif /* _DT_BINDINGS_RESET_CONTROLLER_STIH407 */
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] drivers: reset: stih407: Add softreset, powerdown and picophy controllers

[Patrice Chotard]

Hi Peter

2 minor remarks below

Thanks

On 07/01/2014 04:30 PM, Peter Griffin wrote:
> This patch adds a softreset, powerdown and picophy reset controllers for
> the STiH407 SoC.
>
> With this patch three new devices are registered: -
> 1. st,stih407-powerdown
> 2. st,stih407-softreset
> 3. st,stih407-picophyreset
>
> All three devices use system configuration registers mapped via regmap to
> perform the reset or powerdown. The powerdown controller also has
> an acknowledgement.
>
> A separate picophy reset controller manages the different reset channels within
> the picophy, which have a different polarity to the other system softresets.
> Managing these different picophy softreset channels is necessary to correctly
> handle resuming from CPS when USB2 devices are plugged into the USB3 port.
>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro-qxv4g6HH51o@public.gmane.org>
> Signed-off-by: Peter Griffin <peter.griffin-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>   .../bindings/reset/st,sti-picophyreset.txt         |  41 ++++++
>   drivers/reset/sti/Kconfig                          |   4 +
>   drivers/reset/sti/Makefile                         |   1 +
>   drivers/reset/sti/reset-stih407.c                  | 159 +++++++++++++++++++++
>   .../dt-bindings/reset-controller/stih407-resets.h  |  60 ++++++++
>   5 files changed, 265 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt
>   create mode 100644 drivers/reset/sti/reset-stih407.c
>   create mode 100644 include/dt-bindings/reset-controller/stih407-resets.h
>
> diff --git a/Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt b/Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt
> new file mode 100644
> index 0000000..4c756d1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt
> @@ -0,0 +1,41 @@
> +STMicroelectronics STi family Sysconfig Picophy SoftReset Controller
> +=============================================================================
> +
> +This binding describes a reset controller device that is used to enable and
> +disable on-chip PicoPHY USB2 phy(s) using "softreset" control bits found in
> +the STi family SoC system configuration registers.
> +
> +The actual action taken when softreset is asserted is hardware dependent.
> +However, when asserted it may not be possible to access the hardware's
> +registers and after an assert/deassert sequence the hardware's previous state
> +may no longer be valid.
> +
> +Please refer to Documentation/devicetree/bindings/reset/reset.txt
> +for common reset controller binding usage.
> +
> +Required properties:
> +- compatible: Should be "st,<chip>-softreset"
> +- #reset-cells: 1, see below
> +
> +example:
> +
> +	picophyreset: picophyreset-controller {
> +		#reset-cells = <1>;
> +		compatible = "st,stih407-picophyreset";
> +	};
> +
> +Specifying picophyreset control of devices
> +=======================================
> +
> +Device nodes should specify the reset channel required in their "resets"
> +property, containing a phandle to the picophyreset device node and an
> +index specifying which channel to use, as described in
> +Documentation/devicetree/bindings/reset/reset.txt.
> +
> +example:
> +	usb2_picophy0: usbpicophy@0 {
> +		resets = <&picophyreset STIH407_PICOPHY0_RESET>;
> +	};
> +
> +Macro definitions for the supported reset channels can be found in:
> +include/dt-bindings/reset-controller/stih407-resets.h
> diff --git a/drivers/reset/sti/Kconfig b/drivers/reset/sti/Kconfig
> index 88d2d03..f8c15a3 100644
> --- a/drivers/reset/sti/Kconfig
> +++ b/drivers/reset/sti/Kconfig
> @@ -12,4 +12,8 @@ config STIH416_RESET
>   	bool
>   	select STI_RESET_SYSCFG
>   
> +config STIH407_RESET
> +	bool
> +	select STI_RESET_SYSCFG
> +
>   endif
> diff --git a/drivers/reset/sti/Makefile b/drivers/reset/sti/Makefile
> index be1c976..dc85dfb 100644
> --- a/drivers/reset/sti/Makefile
> +++ b/drivers/reset/sti/Makefile
> @@ -2,3 +2,4 @@ obj-$(CONFIG_STI_RESET_SYSCFG) += reset-syscfg.o
>   
>   obj-$(CONFIG_STIH415_RESET) += reset-stih415.o
>   obj-$(CONFIG_STIH416_RESET) += reset-stih416.o
> +obj-$(CONFIG_STIH407_RESET) += reset-stih407.o
> diff --git a/drivers/reset/sti/reset-stih407.c b/drivers/reset/sti/reset-stih407.c
> new file mode 100644
> index 0000000..1650792
> --- /dev/null
> +++ b/drivers/reset/sti/reset-stih407.c
> @@ -0,0 +1,159 @@
> +/*
> + * Copyright (C) 2013 STMicroelectronics (R&D) Limited
s/2013/2014
> + * Author: Giuseppe Cavallaro <peppe.cavallaro-qxv4g6HH51o@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#include <dt-bindings/reset-controller/stih407-resets.h>
> +
> +#include "reset-syscfg.h"
> +
> +/*
> + * STiH407 Peripheral powerdown definitions.
> + */
> +static const char stih407_core[] = "st,stih407-core-syscfg";
> +static const char stih407_sbc_reg[] = "st,stih407-sbc-reg-syscfg";
> +static const char stih407_lpm[] = "st,stih407-lpm-syscfg";
> +
> +#define STIH407_PDN_0(_bit) \
> +	_SYSCFG_RST_CH(stih407_core, SYSCFG_5000, _bit, SYSSTAT_5500, _bit)
> +#define STIH407_PDN_1(_bit) \
> +	_SYSCFG_RST_CH(stih407_core, SYSCFG_5001, _bit, SYSSTAT_5501, _bit)
> +#define STIH407_PDN_ETH(_bit, _stat) \
> +	_SYSCFG_RST_CH(stih407_sbc_reg, SYSCFG_4032, _bit, SYSSTAT_4520, _stat)
> +
> +/* Powerdown requests control 0 */
> +#define SYSCFG_5000	0x0
> +#define SYSSTAT_5500	0x7d0
> +/* Powerdown requests control 1 (High Speed Links) */
> +#define SYSCFG_5001	0x4
> +#define SYSSTAT_5501	0x7d4
> +
> +/* Ethernet powerdown/status/reset */
> +#define	SYSCFG_4032	0x80
> +#define	SYSSTAT_4520	0x820
> +
> +#define	SYSCFG_4002	0x8
> +
> +static const struct syscfg_reset_channel_data stih407_powerdowns[] = {
> +	[STIH407_EMISS_POWERDOWN] = STIH407_PDN_0(1),
> +	[STIH407_NAND_POWERDOWN] = STIH407_PDN_0(0),
> +	[STIH407_USB3_POWERDOWN] = STIH407_PDN_1(6),
> +	[STIH407_USB2_PORT1_POWERDOWN] = STIH407_PDN_1(5),
> +	[STIH407_USB2_PORT0_POWERDOWN] = STIH407_PDN_1(4),
> +	[STIH407_PCIE1_POWERDOWN] = STIH407_PDN_1(3),
> +	[STIH407_PCIE0_POWERDOWN] = STIH407_PDN_1(2),
> +	[STIH407_SATA1_POWERDOWN] = STIH407_PDN_1(1),
> +	[STIH407_SATA0_POWERDOWN] = STIH407_PDN_1(0),
> +	[STIH407_ETH1_POWERDOWN] = STIH407_PDN_ETH(0, 2),
> +};
> +
> +/* Reset Generator control 0/1 */
> +#define	SYSCFG_5131	0x20c
> +#define	SYSCFG_5132	0x210
> +
> +#define LPM_SYSCFG_1    0x4	/* Softreset IRB & SBC UART */
> +
> +#define STIH407_SRST_CORE(_reg, _bit) \
> +	_SYSCFG_RST_CH_NO_ACK(stih407_core, _reg, _bit)
> +
> +#define STIH407_SRST_SBC(_reg, _bit) \
> +	_SYSCFG_RST_CH_NO_ACK(stih407_sbc_reg, _reg, _bit)
> +
> +#define STIH407_SRST_LPM(_reg, _bit) \
> +	_SYSCFG_RST_CH_NO_ACK(stih407_lpm, _reg, _bit)
> +
> +static const struct syscfg_reset_channel_data stih407_softresets[] = {
> +	[STIH407_ETH1_SOFTRESET] = STIH407_SRST_SBC(SYSCFG_4002, 4),
> +	[STIH407_MMC1_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 3),
> +	[STIH407_USB2_PORT0_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 28),
> +	[STIH407_USB2_PORT1_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 29),
> +	[STIH407_PICOPHY_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 30),
> +	[STIH407_IRB_SOFTRESET] = STIH407_SRST_LPM(LPM_SYSCFG_1, 6),
> +	[STIH407_PCIE0_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 6),
> +	[STIH407_PCIE1_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 15),
> +	[STIH407_SATA0_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 7),
> +	[STIH407_SATA1_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 16),
> +	[STIH407_MIPHY0_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 4),
> +	[STIH407_MIPHY1_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 13),
> +	[STIH407_MIPHY2_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 22),
> +	[STIH407_SATA0_PWR_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 5),
> +	[STIH407_SATA1_PWR_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 14),
> +	[STIH407_DELTA_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 3),
> +	[STIH407_BLITTER_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 10),
> +	[STIH407_HDTVOUT_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 11),
> +	[STIH407_HDQVDP_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 12),
> +	[STIH407_VDP_AUX_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 14),
> +	[STIH407_COMPO_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 15),
> +	[STIH407_HDMI_TX_PHY_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 21),
> +	[STIH407_JPEG_DEC_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 23),
> +	[STIH407_VP8_DEC_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 24),
> +	[STIH407_GPU_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5131, 30),
> +	[STIH407_HVA_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 0),
> +	[STIH407_ERAM_HVA_SOFTRESET] = STIH407_SRST_CORE(SYSCFG_5132, 1),
> +	[STIH407_LPM_SOFTRESET] = STIH407_SRST_SBC(SYSCFG_4002, 2),
> +	[STIH407_KEYSCAN_SOFTRESET] = STIH407_SRST_LPM(LPM_SYSCFG_1, 8),
> +};
> +
> +/* PicoPHY reset/control */
> +#define SYSCFG_5061	0x0f4
> +
> +static const struct syscfg_reset_channel_data stih407_picophyresets[] = {
> +	[STIH407_PICOPHY0_RESET] = STIH407_SRST_CORE(SYSCFG_5061, 5),
> +	[STIH407_PICOPHY1_RESET] = STIH407_SRST_CORE(SYSCFG_5061, 6),
> +	[STIH407_PICOPHY2_RESET] = STIH407_SRST_CORE(SYSCFG_5061, 7),
> +};
> +
> +static struct syscfg_reset_controller_data stih407_powerdown_controller = {
> +	.wait_for_ack = true,
> +	.nr_channels = ARRAY_SIZE(stih407_powerdowns),
> +	.channels = stih407_powerdowns,
> +};
> +
> +static struct syscfg_reset_controller_data stih407_softreset_controller = {
> +	.wait_for_ack = false,
> +	.active_low = true,
> +	.nr_channels = ARRAY_SIZE(stih407_softresets),
> +	.channels = stih407_softresets,
> +};
> +
> +static struct syscfg_reset_controller_data stih407_picophyreset_controller = {
> +	.wait_for_ack = false,
> +	.nr_channels = ARRAY_SIZE(stih407_picophyresets),
> +	.channels = stih407_picophyresets,
> +};
> +
> +
remove extra line
> +static struct of_device_id stih407_reset_match[] = {
> +	{.compatible = "st,stih407-powerdown",
> +	 .data = &stih407_powerdown_controller,},
> +	{.compatible = "st,stih407-softreset",
> +	 .data = &stih407_softreset_controller,},
> +	{.compatible = "st,stih407-picophyreset",
> +	 .data = &stih407_picophyreset_controller,},
> +	{},
> +};
> +
> +static struct platform_driver stih407_reset_driver = {
> +	.probe = syscfg_reset_probe,
> +	.driver = {
> +		   .name = "reset-stih407",
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = stih407_reset_match,
> +		   },
> +};
> +
> +static int __init stih407_reset_init(void)
> +{
> +	return platform_driver_register(&stih407_reset_driver);
> +}
> +
> +arch_initcall(stih407_reset_init);
> diff --git a/include/dt-bindings/reset-controller/stih407-resets.h b/include/dt-bindings/reset-controller/stih407-resets.h
> new file mode 100644
> index 0000000..85448a3
> --- /dev/null
> +++ b/include/dt-bindings/reset-controller/stih407-resets.h
> @@ -0,0 +1,60 @@
> +/*
> + * This header provides constants for the reset controller
> + * based peripheral powerdown requests on the STMicroelectronics
> + * STiH407 SoC.
> + */
> +#ifndef _DT_BINDINGS_RESET_CONTROLLER_STIH407
> +#define _DT_BINDINGS_RESET_CONTROLLER_STIH407
> +
> +/* Powerdown requests control 0 */
> +#define STIH407_EMISS_POWERDOWN		0
> +#define STIH407_NAND_POWERDOWN		1
> +
> +/* Synp GMAC PowerDown */
> +#define	STIH407_ETH1_POWERDOWN		2
> +/* Powerdown requests control 1 */
> +#define STIH407_USB3_POWERDOWN		3
> +#define STIH407_USB2_PORT1_POWERDOWN	4
> +#define STIH407_USB2_PORT0_POWERDOWN	5
> +#define STIH407_PCIE1_POWERDOWN		6
> +#define STIH407_PCIE0_POWERDOWN		7
> +#define STIH407_SATA1_POWERDOWN		8
> +#define STIH407_SATA0_POWERDOWN		9
> +
> +/* Reset defines */
> +#define	STIH407_ETH1_SOFTRESET		0
> +#define	STIH407_MMC1_SOFTRESET		1
> +#define	STIH407_PICOPHY_SOFTRESET	2
> +#define	STIH407_IRB_SOFTRESET		3
> +#define	STIH407_PCIE0_SOFTRESET		4
> +#define	STIH407_PCIE1_SOFTRESET		5
> +#define	STIH407_SATA0_SOFTRESET		6
> +#define	STIH407_SATA1_SOFTRESET		7
> +#define	STIH407_MIPHY0_SOFTRESET	8
> +#define	STIH407_MIPHY1_SOFTRESET	9
> +#define	STIH407_MIPHY2_SOFTRESET	10
> +#define	STIH407_SATA0_PWR_SOFTRESET	11
> +#define	STIH407_SATA1_PWR_SOFTRESET	12
> +#define	STIH407_DELTA_SOFTRESET		13
> +#define	STIH407_BLITTER_SOFTRESET	14
> +#define	STIH407_HDTVOUT_SOFTRESET	15
> +#define	STIH407_HDQVDP_SOFTRESET	16
> +#define	STIH407_VDP_AUX_SOFTRESET	17
> +#define	STIH407_COMPO_SOFTRESET		18
> +#define	STIH407_HDMI_TX_PHY_SOFTRESET	19
> +#define	STIH407_JPEG_DEC_SOFTRESET	20
> +#define	STIH407_VP8_DEC_SOFTRESET	21
> +#define	STIH407_GPU_SOFTRESET		22
> +#define	STIH407_HVA_SOFTRESET		23
> +#define	STIH407_ERAM_HVA_SOFTRESET	24
> +#define	STIH407_LPM_SOFTRESET		25
> +#define	STIH407_KEYSCAN_SOFTRESET	26
> +#define	STIH407_USB2_PORT0_SOFTRESET	27
> +#define	STIH407_USB2_PORT1_SOFTRESET	28
> +
> +/* Picophy reset defines */
> +#define	STIH407_PICOPHY0_RESET	0
> +#define	STIH407_PICOPHY1_RESET	1
> +#define	STIH407_PICOPHY2_RESET	2
> +
> +#endif /* _DT_BINDINGS_RESET_CONTROLLER_STIH407 */

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] drivers: reset: stih407: Add softreset, powerdown and picophy controllers

[Peter Griffin]

Hi Patrice,

On Tue, 01 Jul 2014, Patrice Chotard wrote:
> Hi Peter
> 
> 2 minor remarks below

Thanks for reviewing, both remarks fixed in v2.

Regards,

Peter.

Re: [PATCH 1/3] drivers: reset: stih407: Add softreset, powerdown and picophy controllers

[Lee Jones]

> This patch adds a softreset, powerdown and picophy reset controllers for
> the STiH407 SoC.

s/adds a softreset/adds softreset/

> With this patch three new devices are registered: -
> 1. st,stih407-powerdown
> 2. st,stih407-softreset
> 3. st,stih407-picophyreset
> 
> All three devices use system configuration registers mapped via regmap to
> perform the reset or powerdown. The powerdown controller also has
> an acknowledgement.
> 
> A separate picophy reset controller manages the different reset channels within
> the picophy, which have a different polarity to the other system softresets.
> Managing these different picophy softreset channels is necessary to correctly
> handle resuming from CPS when USB2 devices are plugged into the USB3 port.
> 
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
>  .../bindings/reset/st,sti-picophyreset.txt         |  41 ++++++
>  drivers/reset/sti/Kconfig                          |   4 +
>  drivers/reset/sti/Makefile                         |   1 +
>  drivers/reset/sti/reset-stih407.c                  | 159 +++++++++++++++++++++
>  .../dt-bindings/reset-controller/stih407-resets.h  |  60 ++++++++

Documentation is normally split out as a separate patch.

>  5 files changed, 265 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt
>  create mode 100644 drivers/reset/sti/reset-stih407.c
>  create mode 100644 include/dt-bindings/reset-controller/stih407-resets.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt b/Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt
> new file mode 100644
> index 0000000..4c756d1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/st,sti-picophyreset.txt
> @@ -0,0 +1,41 @@
> +STMicroelectronics STi family Sysconfig Picophy SoftReset Controller
> +=============================================================================
> +
> +This binding describes a reset controller device that is used to enable and
> +disable on-chip PicoPHY USB2 phy(s) using "softreset" control bits found in
> +the STi family SoC system configuration registers.
> +
> +The actual action taken when softreset is asserted is hardware dependent.
> +However, when asserted it may not be possible to access the hardware's
> +registers and after an assert/deassert sequence the hardware's previous state
> +may no longer be valid.
> +
> +Please refer to Documentation/devicetree/bindings/reset/reset.txt
> +for common reset controller binding usage.
> +
> +Required properties:
> +- compatible: Should be "st,<chip>-softreset"

You need to list the possible values here in full.

> +- #reset-cells: 1, see below
> +
> +example:

Nit: s/example/Example

> +
> +	picophyreset: picophyreset-controller {
> +		#reset-cells = <1>;
> +		compatible = "st,stih407-picophyreset";

Nit: Stick the compatible string at the top.

> +	};
> +
> +Specifying picophyreset control of devices
> +=======================================
> +
> +Device nodes should specify the reset channel required in their "resets"
> +property, containing a phandle to the picophyreset device node and an
> +index specifying which channel to use, as described in
> +Documentation/devicetree/bindings/reset/reset.txt.
> +
> +example:

'\n'

> +	usb2_picophy0: usbpicophy@0 {
> +		resets = <&picophyreset STIH407_PICOPHY0_RESET>;
> +	};
> +
> +Macro definitions for the supported reset channels can be found in:
> +include/dt-bindings/reset-controller/stih407-resets.h
> diff --git a/drivers/reset/sti/Kconfig b/drivers/reset/sti/Kconfig
> index 88d2d03..f8c15a3 100644
> --- a/drivers/reset/sti/Kconfig
> +++ b/drivers/reset/sti/Kconfig
> @@ -12,4 +12,8 @@ config STIH416_RESET
>  	bool
>  	select STI_RESET_SYSCFG
>  
> +config STIH407_RESET
> +	bool
> +	select STI_RESET_SYSCFG
> +

No help?

>  endif
> diff --git a/drivers/reset/sti/Makefile b/drivers/reset/sti/Makefile
> index be1c976..dc85dfb 100644
> --- a/drivers/reset/sti/Makefile
> +++ b/drivers/reset/sti/Makefile
> @@ -2,3 +2,4 @@ obj-$(CONFIG_STI_RESET_SYSCFG) += reset-syscfg.o
>  
>  obj-$(CONFIG_STIH415_RESET) += reset-stih415.o
>  obj-$(CONFIG_STIH416_RESET) += reset-stih416.o
> +obj-$(CONFIG_STIH407_RESET) += reset-stih407.o

Genuine question: how different are these?  Can they be consolidated?

> diff --git a/drivers/reset/sti/reset-stih407.c b/drivers/reset/sti/reset-stih407.c
> new file mode 100644
> index 0000000..1650792
> --- /dev/null
> +++ b/drivers/reset/sti/reset-stih407.c
> @@ -0,0 +1,159 @@
> +/*
> + * Copyright (C) 2013 STMicroelectronics (R&D) Limited
> + * Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#include <dt-bindings/reset-controller/stih407-resets.h>
> +
> +#include "reset-syscfg.h"

May as well squash these up.

> +/*
> + * STiH407 Peripheral powerdown definitions.
> + */

Should be single line comment.

> +static const char stih407_core[] = "st,stih407-core-syscfg";
> +static const char stih407_sbc_reg[] = "st,stih407-sbc-reg-syscfg";
> +static const char stih407_lpm[] = "st,stih407-lpm-syscfg";
> +
> +#define STIH407_PDN_0(_bit) \
> +	_SYSCFG_RST_CH(stih407_core, SYSCFG_5000, _bit, SYSSTAT_5500, _bit)
> +#define STIH407_PDN_1(_bit) \
> +	_SYSCFG_RST_CH(stih407_core, SYSCFG_5001, _bit, SYSSTAT_5501, _bit)
> +#define STIH407_PDN_ETH(_bit, _stat) \
> +	_SYSCFG_RST_CH(stih407_sbc_reg, SYSCFG_4032, _bit, SYSSTAT_4520, _stat)
> +
> +/* Powerdown requests control 0 */
> +#define SYSCFG_5000	0x0
> +#define SYSSTAT_5500	0x7d0

Separate these with a '\n'.

> +/* Powerdown requests control 1 (High Speed Links) */
> +#define SYSCFG_5001	0x4
> +#define SYSSTAT_5501	0x7d4
> +
> +/* Ethernet powerdown/status/reset */
> +#define	SYSCFG_4032	0x80
> +#define	SYSSTAT_4520	0x820
> +

What does this '\n' separate? Is it an Ethernet related value, or not?

> +#define	SYSCFG_4002	0x8

Can you address the formatting for all of the above.  Sometimes tabs
are used, other times it's spaces and the padding is also different -
can you standardise to 3 values post-fixing the '0x' i.e. 0xNNN.

> +static const struct syscfg_reset_channel_data stih407_powerdowns[] = {
> +	[STIH407_EMISS_POWERDOWN] = STIH407_PDN_0(1),
> +	[STIH407_NAND_POWERDOWN] = STIH407_PDN_0(0),
> +	[STIH407_USB3_POWERDOWN] = STIH407_PDN_1(6),
> +	[STIH407_USB2_PORT1_POWERDOWN] = STIH407_PDN_1(5),
> +	[STIH407_USB2_PORT0_POWERDOWN] = STIH407_PDN_1(4),
> +	[STIH407_PCIE1_POWERDOWN] = STIH407_PDN_1(3),
> +	[STIH407_PCIE0_POWERDOWN] = STIH407_PDN_1(2),
> +	[STIH407_SATA1_POWERDOWN] = STIH407_PDN_1(1),
> +	[STIH407_SATA0_POWERDOWN] = STIH407_PDN_1(0),
> +	[STIH407_ETH1_POWERDOWN] = STIH407_PDN_ETH(0, 2),
> +};

Being a little OCD, I _personally_ like to see the '='s lined up with
tabs, but some maintainers don't like that.  Philipp's call I guess.

[...]

> +static struct syscfg_reset_controller_data stih407_powerdown_controller = {
> +	.wait_for_ack = true,
> +	.nr_channels = ARRAY_SIZE(stih407_powerdowns),
> +	.channels = stih407_powerdowns,
> +};
> +
> +static struct syscfg_reset_controller_data stih407_softreset_controller = {
> +	.wait_for_ack = false,
> +	.active_low = true,
> +	.nr_channels = ARRAY_SIZE(stih407_softresets),
> +	.channels = stih407_softresets,
> +};
> +
> +static struct syscfg_reset_controller_data stih407_picophyreset_controller = {
> +	.wait_for_ack = false,
> +	.nr_channels = ARRAY_SIZE(stih407_picophyresets),
> +	.channels = stih407_picophyresets,
> +};

I believe these should be const.

> +static struct of_device_id stih407_reset_match[] = {
> +	{.compatible = "st,stih407-powerdown",
> +	 .data = &stih407_powerdown_controller,},
> +	{.compatible = "st,stih407-softreset",
> +	 .data = &stih407_softreset_controller,},
> +	{.compatible = "st,stih407-picophyreset",
> +	 .data = &stih407_picophyreset_controller,},

Formatting should be:

	{
		.compatible = "st,stih407-picophyreset",
	 	.data = &stih407_picophyreset_controller,
	},


> +	{},
> +};
> +
> +static struct platform_driver stih407_reset_driver = {
> +	.probe = syscfg_reset_probe,
> +	.driver = {
> +		   .name = "reset-stih407",
> +		   .owner = THIS_MODULE,

Remove this line, it's done for you as part of
platform_driver_register().

> +		   .of_match_table = stih407_reset_match,
> +		   },

Tabbing is borked.

> +};
> +
> +static int __init stih407_reset_init(void)
> +{
> +	return platform_driver_register(&stih407_reset_driver);
> +}
> +
> +arch_initcall(stih407_reset_init);
> diff --git a/include/dt-bindings/reset-controller/stih407-resets.h b/include/dt-bindings/reset-controller/stih407-resets.h
> new file mode 100644
> index 0000000..85448a3
> --- /dev/null
> +++ b/include/dt-bindings/reset-controller/stih407-resets.h
> @@ -0,0 +1,60 @@
> +/*
> + * This header provides constants for the reset controller
> + * based peripheral powerdown requests on the STMicroelectronics
> + * STiH407 SoC.
> + */
> +#ifndef _DT_BINDINGS_RESET_CONTROLLER_STIH407
> +#define _DT_BINDINGS_RESET_CONTROLLER_STIH407
> +
> +/* Powerdown requests control 0 */
> +#define STIH407_EMISS_POWERDOWN		0
> +#define STIH407_NAND_POWERDOWN		1
> +
> +/* Synp GMAC PowerDown */
> +#define	STIH407_ETH1_POWERDOWN		2

For consistency, either add a line here, or remove the one 3 up.

> +/* Powerdown requests control 1 */
> +#define STIH407_USB3_POWERDOWN		3
> +#define STIH407_USB2_PORT1_POWERDOWN	4
> +#define STIH407_USB2_PORT0_POWERDOWN	5
> +#define STIH407_PCIE1_POWERDOWN		6
> +#define STIH407_PCIE0_POWERDOWN		7
> +#define STIH407_SATA1_POWERDOWN		8
> +#define STIH407_SATA0_POWERDOWN		9

Do these all line up in your editor? 

> +/* Reset defines */
> +#define	STIH407_ETH1_SOFTRESET		0
> +#define	STIH407_MMC1_SOFTRESET		1
> +#define	STIH407_PICOPHY_SOFTRESET	2
> +#define	STIH407_IRB_SOFTRESET		3
> +#define	STIH407_PCIE0_SOFTRESET		4
> +#define	STIH407_PCIE1_SOFTRESET		5
> +#define	STIH407_SATA0_SOFTRESET		6
> +#define	STIH407_SATA1_SOFTRESET		7
> +#define	STIH407_MIPHY0_SOFTRESET	8
> +#define	STIH407_MIPHY1_SOFTRESET	9
> +#define	STIH407_MIPHY2_SOFTRESET	10
> +#define	STIH407_SATA0_PWR_SOFTRESET	11
> +#define	STIH407_SATA1_PWR_SOFTRESET	12
> +#define	STIH407_DELTA_SOFTRESET		13
> +#define	STIH407_BLITTER_SOFTRESET	14
> +#define	STIH407_HDTVOUT_SOFTRESET	15
> +#define	STIH407_HDQVDP_SOFTRESET	16
> +#define	STIH407_VDP_AUX_SOFTRESET	17
> +#define	STIH407_COMPO_SOFTRESET		18
> +#define	STIH407_HDMI_TX_PHY_SOFTRESET	19
> +#define	STIH407_JPEG_DEC_SOFTRESET	20
> +#define	STIH407_VP8_DEC_SOFTRESET	21
> +#define	STIH407_GPU_SOFTRESET		22
> +#define	STIH407_HVA_SOFTRESET		23
> +#define	STIH407_ERAM_HVA_SOFTRESET	24
> +#define	STIH407_LPM_SOFTRESET		25
> +#define	STIH407_KEYSCAN_SOFTRESET	26
> +#define	STIH407_USB2_PORT0_SOFTRESET	27
> +#define	STIH407_USB2_PORT1_SOFTRESET	28

Again, you have tabs here and spaces elsewhere.

I suggest you use a single space everywhere after '#define' and then
line up the values on the right with tabs.

> +/* Picophy reset defines */
> +#define	STIH407_PICOPHY0_RESET	0
> +#define	STIH407_PICOPHY1_RESET	1
> +#define	STIH407_PICOPHY2_RESET	2
> +
> +#endif /* _DT_BINDINGS_RESET_CONTROLLER_STIH407 */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/3] drivers: reset: stih407: Add softreset, powerdown and picophy controllers

[Peter Griffin]

Hi Lee,

Thanks for reviewing, see my inline comments below

> > This patch adds a softreset, powerdown and picophy reset controllers for
> > the STiH407 SoC.
> 
> s/adds a softreset/adds softreset/

Fixed in V2

> 
> >  .../bindings/reset/st,sti-picophyreset.txt         |  41 ++++++
> >  drivers/reset/sti/Kconfig                          |   4 +
> >  drivers/reset/sti/Makefile                         |   1 +
> >  drivers/reset/sti/reset-stih407.c                  | 159 +++++++++++++++++++++
> >  .../dt-bindings/reset-controller/stih407-resets.h  |  60 ++++++++
> 
> Documentation is normally split out as a separate patch.

Ok will seperate out in v2.

> > +Please refer to Documentation/devicetree/bindings/reset/reset.txt
> > +for common reset controller binding usage.
> > +
> > +Required properties:
> > +- compatible: Should be "st,<chip>-softreset"
> 
> You need to list the possible values here in full.

Ok changed in V2
> 
> > +- #reset-cells: 1, see below
> > +
> > +example:
> 
> Nit: s/example/Example

Changed in V2

> 
> > +
> > +	picophyreset: picophyreset-controller {
> > +		#reset-cells = <1>;
> > +		compatible = "st,stih407-picophyreset";
> 
> Nit: Stick the compatible string at the top.

Changed in V2

> > +
> > +example:
> 
> '\n'

Ok Changed in V2, and I capitialized the 'E' whilst I was there.

> 
> > +	usb2_picophy0: usbpicophy@0 {
> > +		resets = <&picophyreset STIH407_PICOPHY0_RESET>;
> > +	};
> > +
> > +Macro definitions for the supported reset channels can be found in:
> > +include/dt-bindings/reset-controller/stih407-resets.h
> > diff --git a/drivers/reset/sti/Kconfig b/drivers/reset/sti/Kconfig
> > index 88d2d03..f8c15a3 100644
> > --- a/drivers/reset/sti/Kconfig
> > +++ b/drivers/reset/sti/Kconfig
> > @@ -12,4 +12,8 @@ config STIH416_RESET
> >  	bool
> >  	select STI_RESET_SYSCFG
> >  
> > +config STIH407_RESET
> > +	bool
> > +	select STI_RESET_SYSCFG
> > +
> 
> No help?

Nope, currently no other platform using reset API has provided help.

> 
> >  endif
> > diff --git a/drivers/reset/sti/Makefile b/drivers/reset/sti/Makefile
> > index be1c976..dc85dfb 100644
> > --- a/drivers/reset/sti/Makefile
> > +++ b/drivers/reset/sti/Makefile
> > @@ -2,3 +2,4 @@ obj-$(CONFIG_STI_RESET_SYSCFG) += reset-syscfg.o
> >  
> >  obj-$(CONFIG_STIH415_RESET) += reset-stih415.o
> >  obj-$(CONFIG_STIH416_RESET) += reset-stih416.o
> > +obj-$(CONFIG_STIH407_RESET) += reset-stih407.o
> 
> Genuine question: how different are these?  Can they be consolidated?

No, the common code is already abstracted into reset-syscfg.c. The SoC specific parts are
in the reset-stiXYZ files.

> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <dt-bindings/reset-controller/stih407-resets.h>
> > +
> > +#include "reset-syscfg.h"
> 
> May as well squash these up.

Fixed in V2, but alters the style versus the other reset-XYZ files in this 
directory.

> 
> > +/*
> > + * STiH407 Peripheral powerdown definitions.
> > + */
> 
> Should be single line comment.

Fixed in V2.
> 
> > +static const char stih407_core[] = "st,stih407-core-syscfg";
> > +static const char stih407_sbc_reg[] = "st,stih407-sbc-reg-syscfg";
> > +static const char stih407_lpm[] = "st,stih407-lpm-syscfg";
> > +
> > +#define STIH407_PDN_0(_bit) \
> > +	_SYSCFG_RST_CH(stih407_core, SYSCFG_5000, _bit, SYSSTAT_5500, _bit)
> > +#define STIH407_PDN_1(_bit) \
> > +	_SYSCFG_RST_CH(stih407_core, SYSCFG_5001, _bit, SYSSTAT_5501, _bit)
> > +#define STIH407_PDN_ETH(_bit, _stat) \
> > +	_SYSCFG_RST_CH(stih407_sbc_reg, SYSCFG_4032, _bit, SYSSTAT_4520, _stat)
> > +
> > +/* Powerdown requests control 0 */
> > +#define SYSCFG_5000	0x0
> > +#define SYSSTAT_5500	0x7d0
> 
> Separate these with a '\n'.

Have fixed in V2
> 
> > +/* Powerdown requests control 1 (High Speed Links) */
> > +#define SYSCFG_5001	0x4
> > +#define SYSSTAT_5501	0x7d4
> > +
> > +/* Ethernet powerdown/status/reset */
> > +#define	SYSCFG_4032	0x80
> > +#define	SYSSTAT_4520	0x820
> > +
> 
> What does this '\n' separate? Is it an Ethernet related value, or not?

Fixed in V2, have removed the '\n'

> 
> > +#define	SYSCFG_4002	0x8
> 
> Can you address the formatting for all of the above.  Sometimes tabs
> are used, other times it's spaces and the padding is also different -

Fixed in V2.

> can you standardise to 3 values post-fixing the '0x' i.e. 0xNNN.

I can change this, but it alters the style versus the other reset-XYZ files
in this directory.

> 
> > +static const struct syscfg_reset_channel_data stih407_powerdowns[] = {
> > +	[STIH407_EMISS_POWERDOWN] = STIH407_PDN_0(1),
> > +	[STIH407_NAND_POWERDOWN] = STIH407_PDN_0(0),
> > +	[STIH407_USB3_POWERDOWN] = STIH407_PDN_1(6),
> > +	[STIH407_USB2_PORT1_POWERDOWN] = STIH407_PDN_1(5),
> > +	[STIH407_USB2_PORT0_POWERDOWN] = STIH407_PDN_1(4),
> > +	[STIH407_PCIE1_POWERDOWN] = STIH407_PDN_1(3),
> > +	[STIH407_PCIE0_POWERDOWN] = STIH407_PDN_1(2),
> > +	[STIH407_SATA1_POWERDOWN] = STIH407_PDN_1(1),
> > +	[STIH407_SATA0_POWERDOWN] = STIH407_PDN_1(0),
> > +	[STIH407_ETH1_POWERDOWN] = STIH407_PDN_ETH(0, 2),
> > +};
> 
> Being a little OCD, I _personally_ like to see the '='s lined up with
> tabs, but some maintainers don't like that.  Philipp's call I guess.

I will leave this one for the maintainer to decide, as maintaining that sort of
alignment can be time consuming.

> > +static struct syscfg_reset_controller_data stih407_picophyreset_controller = {
> > +	.wait_for_ack = false,
> > +	.nr_channels = ARRAY_SIZE(stih407_picophyresets),
> > +	.channels = stih407_picophyresets,
> > +};
> 
> I believe these should be const.

Fixed in V2.

> 
> > +static struct of_device_id stih407_reset_match[] = {
> > +	{.compatible = "st,stih407-powerdown",
> > +	 .data = &stih407_powerdown_controller,},
> > +	{.compatible = "st,stih407-softreset",
> > +	 .data = &stih407_softreset_controller,},
> > +	{.compatible = "st,stih407-picophyreset",
> > +	 .data = &stih407_picophyreset_controller,},
> 
> Formatting should be:
> 
> 	{
> 		.compatible = "st,stih407-picophyreset",
> 	 	.data = &stih407_picophyreset_controller,
> 	},

Changed in V2, but it alters the style versus other reset-XYZ dfiles in this directory.

> > +	{},
> > +};
> > +
> > +static struct platform_driver stih407_reset_driver = {
> > +	.probe = syscfg_reset_probe,
> > +	.driver = {
> > +		   .name = "reset-stih407",
> > +		   .owner = THIS_MODULE,
> 
> Remove this line, it's done for you as part of
> platform_driver_register().

Fixed in V2.

> 
> > +		   .of_match_table = stih407_reset_match,
> > +		   },
> 
> Tabbing is borked.

Fixed in V2

> > +/* Synp GMAC PowerDown */
> > +#define	STIH407_ETH1_POWERDOWN		2
> 
> For consistency, either add a line here, or remove the one 3 up.

Fixed in V2
> 
> > +/* Powerdown requests control 1 */
> > +#define STIH407_USB3_POWERDOWN		3
> > +#define STIH407_USB2_PORT1_POWERDOWN	4
> > +#define STIH407_USB2_PORT0_POWERDOWN	5
> > +#define STIH407_PCIE1_POWERDOWN		6
> > +#define STIH407_PCIE0_POWERDOWN		7
> > +#define STIH407_SATA1_POWERDOWN		8
> > +#define STIH407_SATA0_POWERDOWN		9
> 
> Do these all line up in your editor? 

Yes
> 
> > +#define	STIH407_KEYSCAN_SOFTRESET	26
> > +#define	STIH407_USB2_PORT0_SOFTRESET	27
> > +#define	STIH407_USB2_PORT1_SOFTRESET	28
> 
> Again, you have tabs here and spaces elsewhere.

Fixed in V2

regards,

Peter.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] ARM: sti: Add STiH407 Kconfig entry to select STIH407_RESET

[Peter Griffin]

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 arch/arm/mach-sti/Kconfig | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/mach-sti/Kconfig b/arch/arm/mach-sti/Kconfig
index 878e9ec..8825bc9 100644
--- a/arch/arm/mach-sti/Kconfig
+++ b/arch/arm/mach-sti/Kconfig
@@ -42,4 +42,14 @@ config SOC_STIH416
 	  and other digital audio/video applications using Flattened Device
 	  Trees.
 
+config SOC_STIH407
+	bool "STiH407 STMicroelectronics Consumer Electronics family"
+	default y
+	select STIH407_RESET
+	help
+	  This enables support for STMicroelectronics Digital Consumer
+	  Electronics family StiH407 parts, targetted at set-top-box
+	  and other digital audio/video applications using Flattened Device
+	  Trees.
+
 endif
-- 
1.9.1

Re: [PATCH 2/3] ARM: sti: Add STiH407 Kconfig entry to select STIH407_RESET

[Lee Jones]

On Tue, 01 Jul 2014, Peter Griffin wrote:

> Signed-off-by: Peter Griffin <peter.griffin-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/mach-sti/Kconfig | 10 ++++++++++
>  1 file changed, 10 insertions(+)

Looks okay, although could probably do with a few words in the commit
message about what the STiH407 is; yada, yada, yada ...

Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

> diff --git a/arch/arm/mach-sti/Kconfig b/arch/arm/mach-sti/Kconfig
> index 878e9ec..8825bc9 100644
> --- a/arch/arm/mach-sti/Kconfig
> +++ b/arch/arm/mach-sti/Kconfig
> @@ -42,4 +42,14 @@ config SOC_STIH416
>  	  and other digital audio/video applications using Flattened Device
>  	  Trees.
>  
> +config SOC_STIH407
> +	bool "STiH407 STMicroelectronics Consumer Electronics family"
> +	default y
> +	select STIH407_RESET
> +	help
> +	  This enables support for STMicroelectronics Digital Consumer
> +	  Electronics family StiH407 parts, targetted at set-top-box
> +	  and other digital audio/video applications using Flattened Device
> +	  Trees.
> +
>  endif

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] ARM: sti: Add STiH407 Kconfig entry to select STIH407_RESET

[Peter Griffin]

Hi Lee,

Thanks for reviewing.

On Wed, 02 Jul 2014, Lee Jones wrote:
> Looks okay, although could probably do with a few words in the commit
> message about what the STiH407 is; yada, yada, yada ...

Fixed in V2.
> 
> Acked-by: Lee Jones <lee.jones@linaro.org>

Thanks, regards,

Peter.

[PATCH 3/3] ARM: STi: STiH407: Add reset controller support.

[Peter Griffin]

This patch adds the reset controller DT nodes for the powerdown,
 softreset and picophy controllers.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 arch/arm/boot/dts/stih407.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/stih407.dtsi b/arch/arm/boot/dts/stih407.dtsi
index 4f9024f..e8fbb88 100644
--- a/arch/arm/boot/dts/stih407.dtsi
+++ b/arch/arm/boot/dts/stih407.dtsi
@@ -8,6 +8,7 @@
  */
 #include "stih407-clock.dtsi"
 #include "stih407-pinctrl.dtsi"
+#include <dt-bindings/reset-controller/stih407-resets.h>
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
@@ -63,6 +64,21 @@
 		ranges;
 		compatible = "simple-bus";
 
+		powerdown: powerdown-controller {
+			#reset-cells = <1>;
+			compatible = "st,stih407-powerdown";
+		};
+
+		softreset: softreset-controller {
+			#reset-cells = <1>;
+			compatible = "st,stih407-softreset";
+		};
+
+		picophyreset: picophyreset-controller {
+			#reset-cells = <1>;
+			compatible = "st,stih407-picophyreset";
+		};
+
 		syscfg_sbc: sbc-syscfg@9620000 {
 			compatible = "st,stih407-sbc-syscfg", "syscon";
 			reg = <0x9620000 0x1000>;
-- 
1.9.1

Re: [PATCH 3/3] ARM: STi: STiH407: Add reset controller support.

[Lee Jones]

On Tue, 01 Jul 2014, Peter Griffin wrote:

> This patch adds the reset controller DT nodes for the powerdown,
>  softreset and picophy controllers.
> 
> Signed-off-by: Peter Griffin <peter.griffin-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm/boot/dts/stih407.dtsi | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stih407.dtsi b/arch/arm/boot/dts/stih407.dtsi
> index 4f9024f..e8fbb88 100644
> --- a/arch/arm/boot/dts/stih407.dtsi
> +++ b/arch/arm/boot/dts/stih407.dtsi
> @@ -8,6 +8,7 @@
>   */
>  #include "stih407-clock.dtsi"
>  #include "stih407-pinctrl.dtsi"
> +#include <dt-bindings/reset-controller/stih407-resets.h>
>  / {
>  	#address-cells = <1>;
>  	#size-cells = <1>;
> @@ -63,6 +64,21 @@
>  		ranges;
>  		compatible = "simple-bus";
>  
> +		powerdown: powerdown-controller {
> +			#reset-cells = <1>;
> +			compatible = "st,stih407-powerdown";
> +		};
> +
> +		softreset: softreset-controller {
> +			#reset-cells = <1>;
> +			compatible = "st,stih407-softreset";
> +		};
> +
> +		picophyreset: picophyreset-controller {
> +			#reset-cells = <1>;
> +			compatible = "st,stih407-picophyreset";
> +		};
> +

I'd always put the compatible string at the very top of the node.

>  		syscfg_sbc: sbc-syscfg@9620000 {
>  			compatible = "st,stih407-sbc-syscfg", "syscon";
>  			reg = <0x9620000 0x1000>;

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] ARM: STi: STiH407: Add reset controller support.

[Peter Griffin]

Hi Lee,

> > +		picophyreset: picophyreset-controller {
> > +			#reset-cells = <1>;
> > +			compatible = "st,stih407-picophyreset";
> > +		};
> > +
> 
> I'd always put the compatible string at the very top of the node.

Thanks for reviewing, have changed in v2.

regards,

Peter.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html