diff mbox

[v2] reset: Add i.MX7 SRC reset driver

Message ID 20170213153338.13518-1-andrew.smirnov@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andrey Smirnov Feb. 13, 2017, 3:33 p.m. UTC
This driver exposes various reset faculties, impelented by System Reset
Controller IP block, as a reset driver. Currently only PCIE related
reset lines are implemented.

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---

Changes since v1 (see [v1]):

	- Various small DT bindings description fixes as per feedback
          from Rob Herring


[v1] https://lkml.org/lkml/2017/2/6/554

 .../devicetree/bindings/reset/fsl,imx7-src.txt     |  47 +++++++
 drivers/reset/Kconfig                              |   8 ++
 drivers/reset/Makefile                             |   1 +
 drivers/reset/reset-imx7.c                         | 149 +++++++++++++++++++++
 include/dt-bindings/reset/imx7-reset.h             |  28 ++++
 5 files changed, 233 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
 create mode 100644 drivers/reset/reset-imx7.c
 create mode 100644 include/dt-bindings/reset/imx7-reset.h

Comments

Philipp Zabel Feb. 13, 2017, 4:50 p.m. UTC | #1
Hi Andrey,

thank you for the driver, I have a few comments below.

On Mon, 2017-02-13 at 07:33 -0800, Andrey Smirnov wrote:
> This driver exposes various reset faculties, impelented by System Reset

s/impelented/implemented/

> Controller IP block, as a reset driver. Currently only PCIE related
> reset lines are implemented.
> 
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> 
> Changes since v1 (see [v1]):
> 
> 	- Various small DT bindings description fixes as per feedback
>           from Rob Herring
> 
> 
> [v1] https://lkml.org/lkml/2017/2/6/554
> 
>  .../devicetree/bindings/reset/fsl,imx7-src.txt     |  47 +++++++
>  drivers/reset/Kconfig                              |   8 ++
>  drivers/reset/Makefile                             |   1 +
>  drivers/reset/reset-imx7.c                         | 149 +++++++++++++++++++++
>  include/dt-bindings/reset/imx7-reset.h             |  28 ++++
>  5 files changed, 233 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
>  create mode 100644 drivers/reset/reset-imx7.c
>  create mode 100644 include/dt-bindings/reset/imx7-reset.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
> new file mode 100644
> index 0000000..c38f7f0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
> @@ -0,0 +1,47 @@
> +Freescale i.MX7 System Reset Controller
> +======================================
> +
> +Please also refer to reset.txt in this directory for common reset
> +controller binding usage.
> +
> +Required properties:
> +- compatible: Should be "fsl,imx7-src", "syscon"
> +- reg: should be register base and length as documented in the
> +  datasheet
> +- interrupts: Should contain SRC interrupt
> +- #reset-cells: 1, see below
> +
> +example:
> +
> +src: reset-controller@30390000 {
> +     compatible = "fsl,imx7d-src", "syscon";
> +     reg = <0x30390000 0x2000>;
> +     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
> +     #reset-cells = <1>;
> +};
> +
> +
> +Specifying reset lines connected to IP modules
> +==============================================
> +
> +The system reset controller can be used to reset various set of
> +peripherals. Device nodes that need access to reset lines should
> +specify them as a reset phandle in their corresponding node as
> +specified in reset.txt.
> +
> +Example:
> +
> +	pcie: pcie@33800000 {
> +
> +		...
> +
> +		resets = <&src IMX7_RESET_PCIEPHY>,
> +			 <&src IMX7_RESET_PCIE_CTRL_APPS>;
> +		reset-names = "pciephy", "apps";
> +
> +		...
> +        };
> +
> +
> +For list of all valid reset indicies see
> +<dt-bindings/reset/imx7-reset.h>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 172dc96..664a88b 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -92,6 +92,14 @@ config RESET_ZYNQ
>  	help
>  	  This enables the reset controller driver for Xilinx Zynq SoCs.
>  
> +config RESET_IMX7
> +	bool "i.MX7 Reset Driver"
> +	depends on SOC_IMX7D || COMPILE_TEST
> +	select MFD_SYSCON
> +	default SOC_IMX7D
> +	help
> +	  This enables the reset controller driver for i.MX7 SoCs.
> +

Alphabetical order, please.

>  source "drivers/reset/sti/Kconfig"
>  source "drivers/reset/hisilicon/Kconfig"
>  source "drivers/reset/tegra/Kconfig"
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 13b346e..417c9d0 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
>  obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o
>  obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
>  obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
> +obj-$(CONFIG_RESET_IMX7) += reset-imx7.o

See above, those are also in alphabetical order.

> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> new file mode 100644
> index 0000000..6d90e57
> --- /dev/null
> +++ b/drivers/reset/reset-imx7.c
> @@ -0,0 +1,149 @@
> +/*
> + * Copyright (c) 2017, Impinj, Inc.
> + *
> + * i.MX7 System Reset Controller (SRC) driver
> + *
> + * Author: Andrey Smirnov <andrew.smirnov@gmail.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; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>

I think this is not be necessary because register access is done via
regmap. Please cull unnecessary headers.

> +#include <linux/init.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <dt-bindings/reset/imx7-reset.h>
> +
> +struct imx7_src {
> +	struct reset_controller_dev rcdev;
> +	struct device *dev;
> +	struct regmap *regmap;
> +};
> +
> +enum imx7_src_registers {
> +	SRC_PCIEPHY_RCR = 0x002c,
> +	SRC_PCIEPHY_RCR_PCIEPHY_G_RST     = BIT(1),

What about the others resets? There's at least HSICPHY, MIPIPHY and
USBOPHY registers before the PCIEPHY register.

> +	SRC_PCIEPHY_RCR_PCIEPHY_BTN       = BIT(2),
> +	SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN = BIT(6),

Are those really resets? At least the PCIE_CTRL_APPS_EN has a bit called
PCIE_CTRL_APPS_RST right next to it, so this warrants some explanation.

> +};
> +
> +static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
> +{
> +	return container_of(rcdev, struct imx7_src, rcdev);
> +}
> +
> +static int imx7_reset_assert(struct reset_controller_dev *rcdev,
> +			     unsigned long id)
> +{
> +	struct imx7_src *imx7src = to_imx7_src(rcdev);
> +
> +	switch (id) {
> +	case IMX7_RESET_PCIE_CTRL_APPS:
> +		regmap_update_bits(imx7src->regmap,
> +				   SRC_PCIEPHY_RCR,
> +				   SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN, 0);
> +		return 0;
> +
> +	case IMX7_RESET_PCIEPHY:
> +		regmap_update_bits(imx7src->regmap,
> +				   SRC_PCIEPHY_RCR,
> +				   SRC_PCIEPHY_RCR_PCIEPHY_G_RST,
> +				   SRC_PCIEPHY_RCR_PCIEPHY_G_RST);
> +		regmap_update_bits(imx7src->regmap,
> +				   SRC_PCIEPHY_RCR,
> +				   SRC_PCIEPHY_RCR_PCIEPHY_BTN,
> +				   SRC_PCIEPHY_RCR_PCIEPHY_BTN);

What is the PCIE PHY button, and why does it have to be set with the
reset bit?

> +		return 0;
> +	default:
> +		dev_err(imx7src->dev, "Unknown reset ID %lu\n", id);
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int imx7_reset_deassert(struct reset_controller_dev *rcdev,
> +			       unsigned long id)
> +{
> +	struct imx7_src *imx7src = to_imx7_src(rcdev);
> +
> +	switch (id) {
> +	case IMX7_RESET_PCIE_CTRL_APPS:
> +		regmap_update_bits(imx7src->regmap,
> +				   SRC_PCIEPHY_RCR,
> +				   SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN,
> +				   SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN);
> +		return 0;
> +
> +	case IMX7_RESET_PCIEPHY:
> +		/* wait for more than 10us to release phy g_rst and btnrst */
> +		udelay(10);
> +		regmap_update_bits(imx7src->regmap,
> +				   SRC_PCIEPHY_RCR,
> +				   SRC_PCIEPHY_RCR_PCIEPHY_G_RST, 0);
> +		regmap_update_bits(imx7src->regmap,
> +				   SRC_PCIEPHY_RCR,
> +				   SRC_PCIEPHY_RCR_PCIEPHY_BTN, 0);
> +		return 0;
> +	default:
> +		dev_err(imx7src->dev, "Unknown reset ID %lu\n", id);
> +		break;
> +	};
> +
> +	return -EINVAL;
> +}
> +
> +static const struct reset_control_ops imx7_reset_ops = {
> +	.assert		= imx7_reset_assert,
> +	.deassert	= imx7_reset_deassert,
> +};
> +
> +static int imx7_reset_probe(struct platform_device *pdev)
> +{
> +	struct imx7_src *imx7src;
> +	struct device *dev = &pdev->dev;
> +
> +	imx7src = devm_kzalloc(dev, sizeof(*imx7src), GFP_KERNEL);
> +	if (!imx7src)
> +		return -ENOMEM;
> +
> +	imx7src->dev = dev;
> +	imx7src->regmap = syscon_node_to_regmap(dev->of_node);
> +	if (IS_ERR(imx7src->regmap)) {
> +		dev_err(dev, "Unable to get imx7-src regmap");
> +		return PTR_ERR(imx7src->regmap);
> +	}

You could use regmap_attach_dev to attach the device to the regmap. This
would improve error messages, regmap debugfs structure, and make
imx7src->dev unnecessary.

> +
> +	imx7src->rcdev.owner     = THIS_MODULE;
> +	imx7src->rcdev.nr_resets = IMX7_RESET_NUM;
> +	imx7src->rcdev.ops       = &imx7_reset_ops;
> +	imx7src->rcdev.of_node   = dev->of_node;
> +
> +	return devm_reset_controller_register(dev, &imx7src->rcdev);
> +}
> +
> +static const struct of_device_id imx7_reset_dt_ids[] = {
> +	{ .compatible = "fsl,imx7d-src", },
> +	{ /* sentinel */ },
> +};
> +
> +static struct platform_driver imx7_reset_driver = {
> +	.probe	= imx7_reset_probe,
> +	.driver = {
> +		.name		= KBUILD_MODNAME,
> +		.of_match_table	= imx7_reset_dt_ids,
> +	},
> +};
> +builtin_platform_driver(imx7_reset_driver);
> diff --git a/include/dt-bindings/reset/imx7-reset.h b/include/dt-bindings/reset/imx7-reset.h
> new file mode 100644
> index 0000000..e20f2db
> --- /dev/null
> +++ b/include/dt-bindings/reset/imx7-reset.h
> @@ -0,0 +1,28 @@
> +/*
> + * Copyright (C) 2017 Impinj, Inc.
> + *
> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef DT_BINDING_RESET_IMX7_H
> +#define DT_BINDING_RESET_IMX7_H
> +
> +#define IMX7_RESET_PCIE_CTRL_APPS	0
> +#define IMX7_RESET_PCIEPHY		1

It would expect these to be numbered in the order they appear in the
register map, not starting from the end. Could you add all available
peripheral resets to this list, in the correct order?

> +#define IMX7_RESET_NUM			2
> +
> +#endif
> +

regards
Philipp
Andrey Smirnov Feb. 14, 2017, 3:46 p.m. UTC | #2
On Mon, Feb 13, 2017 at 8:50 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hi Andrey,
>
> thank you for the driver, I have a few comments below.
>
> On Mon, 2017-02-13 at 07:33 -0800, Andrey Smirnov wrote:
>> This driver exposes various reset faculties, impelented by System Reset
>
> s/impelented/implemented/

Thanks for noticing, will fix in v3.

>
>> Controller IP block, as a reset driver. Currently only PCIE related
>> reset lines are implemented.
>>
>> Cc: Lucas Stach <l.stach@pengutronix.de>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
>> ---
>>
>> Changes since v1 (see [v1]):
>>
>>       - Various small DT bindings description fixes as per feedback
>>           from Rob Herring
>>
>>
>> [v1] https://lkml.org/lkml/2017/2/6/554
>>
>>  .../devicetree/bindings/reset/fsl,imx7-src.txt     |  47 +++++++
>>  drivers/reset/Kconfig                              |   8 ++
>>  drivers/reset/Makefile                             |   1 +
>>  drivers/reset/reset-imx7.c                         | 149 +++++++++++++++++++++
>>  include/dt-bindings/reset/imx7-reset.h             |  28 ++++
>>  5 files changed, 233 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
>>  create mode 100644 drivers/reset/reset-imx7.c
>>  create mode 100644 include/dt-bindings/reset/imx7-reset.h
>>
>> diff --git a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
>> new file mode 100644
>> index 0000000..c38f7f0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
>> @@ -0,0 +1,47 @@
>> +Freescale i.MX7 System Reset Controller
>> +======================================
>> +
>> +Please also refer to reset.txt in this directory for common reset
>> +controller binding usage.
>> +
>> +Required properties:
>> +- compatible: Should be "fsl,imx7-src", "syscon"
>> +- reg: should be register base and length as documented in the
>> +  datasheet
>> +- interrupts: Should contain SRC interrupt
>> +- #reset-cells: 1, see below
>> +
>> +example:
>> +
>> +src: reset-controller@30390000 {
>> +     compatible = "fsl,imx7d-src", "syscon";
>> +     reg = <0x30390000 0x2000>;
>> +     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
>> +     #reset-cells = <1>;
>> +};
>> +
>> +
>> +Specifying reset lines connected to IP modules
>> +==============================================
>> +
>> +The system reset controller can be used to reset various set of
>> +peripherals. Device nodes that need access to reset lines should
>> +specify them as a reset phandle in their corresponding node as
>> +specified in reset.txt.
>> +
>> +Example:
>> +
>> +     pcie: pcie@33800000 {
>> +
>> +             ...
>> +
>> +             resets = <&src IMX7_RESET_PCIEPHY>,
>> +                      <&src IMX7_RESET_PCIE_CTRL_APPS>;
>> +             reset-names = "pciephy", "apps";
>> +
>> +             ...
>> +        };
>> +
>> +
>> +For list of all valid reset indicies see
>> +<dt-bindings/reset/imx7-reset.h>
>> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
>> index 172dc96..664a88b 100644
>> --- a/drivers/reset/Kconfig
>> +++ b/drivers/reset/Kconfig
>> @@ -92,6 +92,14 @@ config RESET_ZYNQ
>>       help
>>         This enables the reset controller driver for Xilinx Zynq SoCs.
>>
>> +config RESET_IMX7
>> +     bool "i.MX7 Reset Driver"
>> +     depends on SOC_IMX7D || COMPILE_TEST
>> +     select MFD_SYSCON
>> +     default SOC_IMX7D
>> +     help
>> +       This enables the reset controller driver for i.MX7 SoCs.
>> +
>
> Alphabetical order, please.

OK, will fix in v3.

>
>>  source "drivers/reset/sti/Kconfig"
>>  source "drivers/reset/hisilicon/Kconfig"
>>  source "drivers/reset/tegra/Kconfig"
>> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
>> index 13b346e..417c9d0 100644
>> --- a/drivers/reset/Makefile
>> +++ b/drivers/reset/Makefile
>> @@ -14,3 +14,4 @@ obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
>>  obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o
>>  obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
>>  obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
>> +obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
>
> See above, those are also in alphabetical order.

Ditto.

>
>> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
>> new file mode 100644
>> index 0000000..6d90e57
>> --- /dev/null
>> +++ b/drivers/reset/reset-imx7.c
>> @@ -0,0 +1,149 @@
>> +/*
>> + * Copyright (c) 2017, Impinj, Inc.
>> + *
>> + * i.MX7 System Reset Controller (SRC) driver
>> + *
>> + * Author: Andrey Smirnov <andrew.smirnov@gmail.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; version 2 of the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>
> I think this is not be necessary because register access is done via
> regmap. Please cull unnecessary headers.

OK, will do for v3.

>
>> +#include <linux/init.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/reset-controller.h>
>> +#include <linux/regmap.h>
>> +#include <linux/types.h>
>> +#include <dt-bindings/reset/imx7-reset.h>
>> +
>> +struct imx7_src {
>> +     struct reset_controller_dev rcdev;
>> +     struct device *dev;
>> +     struct regmap *regmap;
>> +};
>> +
>> +enum imx7_src_registers {
>> +     SRC_PCIEPHY_RCR = 0x002c,
>> +     SRC_PCIEPHY_RCR_PCIEPHY_G_RST     = BIT(1),
>
> What about the others resets? There's at least HSICPHY, MIPIPHY and
> USBOPHY registers before the PCIEPHY register.

I don't really have any code using anything but PCI reset related
functionality, so I can't test any of the resets you mention. In light
of that I didn't want to add any of the definitions that are not going
to be used anywhere in the code.

>
>> +     SRC_PCIEPHY_RCR_PCIEPHY_BTN       = BIT(2),
>> +     SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN = BIT(6),
>
> Are those really resets? At least the PCIE_CTRL_APPS_EN has a bit called
> PCIE_CTRL_APPS_RST right next to it, so this warrants some explanation.
>

Public documentation for that aspect of i.MX7 is nonexistent and,
unfortunately, that is my only source of information. Given that, I
can't really tell you what the difference between PCIE_CTRL_APPS_EN
and PCIE_CTRL_APPS_RST besides that the former is what downstream PCIe
driver uses to inhibit LTSSM and the latter is not referenced or used
by any code (as far as I am aware).

>> +};
>> +
>> +static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
>> +{
>> +     return container_of(rcdev, struct imx7_src, rcdev);
>> +}
>> +
>> +static int imx7_reset_assert(struct reset_controller_dev *rcdev,
>> +                          unsigned long id)
>> +{
>> +     struct imx7_src *imx7src = to_imx7_src(rcdev);
>> +
>> +     switch (id) {
>> +     case IMX7_RESET_PCIE_CTRL_APPS:
>> +             regmap_update_bits(imx7src->regmap,
>> +                                SRC_PCIEPHY_RCR,
>> +                                SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN, 0);
>> +             return 0;
>> +
>> +     case IMX7_RESET_PCIEPHY:
>> +             regmap_update_bits(imx7src->regmap,
>> +                                SRC_PCIEPHY_RCR,
>> +                                SRC_PCIEPHY_RCR_PCIEPHY_G_RST,
>> +                                SRC_PCIEPHY_RCR_PCIEPHY_G_RST);
>> +             regmap_update_bits(imx7src->regmap,
>> +                                SRC_PCIEPHY_RCR,
>> +                                SRC_PCIEPHY_RCR_PCIEPHY_BTN,
>> +                                SRC_PCIEPHY_RCR_PCIEPHY_BTN);
>
> What is the PCIE PHY button, and why does it have to be set with the
> reset bit?

I am sorry, but just as above, I wouldn't be able to enlighten you any
more about the subject. I have no knowledge about the details of G_RST
and BTN signal (or even if BTN stands for "button") behavior beyond
the fact that that is how downstream driver performs PCIEPHY reset.

>
>> +             return 0;
>> +     default:
>> +             dev_err(imx7src->dev, "Unknown reset ID %lu\n", id);
>> +             break;
>> +     }
>> +
>> +     return -EINVAL;
>> +}
>> +
>> +static int imx7_reset_deassert(struct reset_controller_dev *rcdev,
>> +                            unsigned long id)
>> +{
>> +     struct imx7_src *imx7src = to_imx7_src(rcdev);
>> +
>> +     switch (id) {
>> +     case IMX7_RESET_PCIE_CTRL_APPS:
>> +             regmap_update_bits(imx7src->regmap,
>> +                                SRC_PCIEPHY_RCR,
>> +                                SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN,
>> +                                SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN);
>> +             return 0;
>> +
>> +     case IMX7_RESET_PCIEPHY:
>> +             /* wait for more than 10us to release phy g_rst and btnrst */
>> +             udelay(10);
>> +             regmap_update_bits(imx7src->regmap,
>> +                                SRC_PCIEPHY_RCR,
>> +                                SRC_PCIEPHY_RCR_PCIEPHY_G_RST, 0);
>> +             regmap_update_bits(imx7src->regmap,
>> +                                SRC_PCIEPHY_RCR,
>> +                                SRC_PCIEPHY_RCR_PCIEPHY_BTN, 0);
>> +             return 0;
>> +     default:
>> +             dev_err(imx7src->dev, "Unknown reset ID %lu\n", id);
>> +             break;
>> +     };
>> +
>> +     return -EINVAL;
>> +}
>> +
>> +static const struct reset_control_ops imx7_reset_ops = {
>> +     .assert         = imx7_reset_assert,
>> +     .deassert       = imx7_reset_deassert,
>> +};
>> +
>> +static int imx7_reset_probe(struct platform_device *pdev)
>> +{
>> +     struct imx7_src *imx7src;
>> +     struct device *dev = &pdev->dev;
>> +
>> +     imx7src = devm_kzalloc(dev, sizeof(*imx7src), GFP_KERNEL);
>> +     if (!imx7src)
>> +             return -ENOMEM;
>> +
>> +     imx7src->dev = dev;
>> +     imx7src->regmap = syscon_node_to_regmap(dev->of_node);
>> +     if (IS_ERR(imx7src->regmap)) {
>> +             dev_err(dev, "Unable to get imx7-src regmap");
>> +             return PTR_ERR(imx7src->regmap);
>> +     }
>
> You could use regmap_attach_dev to attach the device to the regmap. This
> would improve error messages, regmap debugfs structure, and make
> imx7src->dev unnecessary.

OK, will do that in v3.

>
>> +
>> +     imx7src->rcdev.owner     = THIS_MODULE;
>> +     imx7src->rcdev.nr_resets = IMX7_RESET_NUM;
>> +     imx7src->rcdev.ops       = &imx7_reset_ops;
>> +     imx7src->rcdev.of_node   = dev->of_node;
>> +
>> +     return devm_reset_controller_register(dev, &imx7src->rcdev);
>> +}
>> +
>> +static const struct of_device_id imx7_reset_dt_ids[] = {
>> +     { .compatible = "fsl,imx7d-src", },
>> +     { /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver imx7_reset_driver = {
>> +     .probe  = imx7_reset_probe,
>> +     .driver = {
>> +             .name           = KBUILD_MODNAME,
>> +             .of_match_table = imx7_reset_dt_ids,
>> +     },
>> +};
>> +builtin_platform_driver(imx7_reset_driver);
>> diff --git a/include/dt-bindings/reset/imx7-reset.h b/include/dt-bindings/reset/imx7-reset.h
>> new file mode 100644
>> index 0000000..e20f2db
>> --- /dev/null
>> +++ b/include/dt-bindings/reset/imx7-reset.h
>> @@ -0,0 +1,28 @@
>> +/*
>> + * Copyright (C) 2017 Impinj, Inc.
>> + *
>> + * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#ifndef DT_BINDING_RESET_IMX7_H
>> +#define DT_BINDING_RESET_IMX7_H
>> +
>> +#define IMX7_RESET_PCIE_CTRL_APPS    0
>> +#define IMX7_RESET_PCIEPHY           1
>
> It would expect these to be numbered in the order they appear in the
> register map, not starting from the end. Could you add all available
> peripheral resets to this list, in the correct order?

The numbering is just a consequence of me adding only resets I could
exercise with my code and numbering then starting from zero. I also
was hesitant adding more sources since it seemed to me that some of
less trivial registers in that IP block might be best represented as a
single reset source doing something more sophisticated that just
setting a bit under the hood.

Thanks,
Andrey Smirnov
Philipp Zabel Feb. 14, 2017, 4:31 p.m. UTC | #3
On Tue, 2017-02-14 at 07:46 -0800, Andrey Smirnov wrote:
[...]
> >> +enum imx7_src_registers {
> >> +     SRC_PCIEPHY_RCR = 0x002c,
> >> +     SRC_PCIEPHY_RCR_PCIEPHY_G_RST     = BIT(1),
> >
> > What about the others resets? There's at least HSICPHY, MIPIPHY and
> > USBOPHY registers before the PCIEPHY register.
> 
> I don't really have any code using anything but PCI reset related
> functionality, so I can't test any of the resets you mention. In light
> of that I didn't want to add any of the definitions that are not going
> to be used anywhere in the code.

Since the device tree bindings are supposed to be descriptive of the
hardware, not of the development history of the linux driver, I'd like
the reset controls to be in a sensible order. From a quick scan of the
chapter in the reference manual, this is the complete list of resets:

A7_CORE_POR_RESET0
A7_CORE_POR_RESET1
A7_CORE_RESET0
A7_CORE_RESET1
A7_DBG_RESET0
A7_DBG_RESET1
A7_ETM_RESET0
A7_ETM_RESET1
A7_SOC_DBG_RESET
A7_L2RESET
SW_M4C_RST
SW_M4P_RST
EIM_RST
HSICPHY_PORT_RST
HSIC_PHY_POR (or Reserved, the manual is conflicted about this)
USBPHY1_POR
USBPHY1_PORT_RST
USBPHY2_POR
USBPHY2_PORT_RST
MIPI_PHY_MRST
MIPI_PHY_SRST
PCIEPHY_G_RST
PCIEPHY_BTN
PCIEPHY_PERST
PCIE_CTRL_APPS_RST/EN

Arguably, the A7 resets should not be handled by the peripheral reset
controller, but at least for the others I see no reason not to leave
space for them in the index table. In fact, since unused reset controls
don't use space, why not number them all?

> >
> >> +     SRC_PCIEPHY_RCR_PCIEPHY_BTN       = BIT(2),
> >> +     SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN = BIT(6),
> >
> > Are those really resets? At least the PCIE_CTRL_APPS_EN has a bit called
> > PCIE_CTRL_APPS_RST right next to it, so this warrants some explanation.
>
> Public documentation for that aspect of i.MX7 is nonexistent and,
> unfortunately, that is my only source of information. Given that, I
> can't really tell you what the difference between PCIE_CTRL_APPS_EN
> and PCIE_CTRL_APPS_RST besides that the former is what downstream PCIe
> driver uses to inhibit LTSSM and the latter is not referenced or used
> by any code (as far as I am aware).

Ok. That's unfortunate, but nothing we can do about that.
After discussing with Lucas, I've come to the belief that APPS_EN stops
the LTSSM, whereas APPS_RST might reset it into the initial state. Since
the latter doesn't exist at all on i.MX6, and we can obviously live
without it, I'm fine with just calling the enable bit an active low
reset to hold it in place. It doesn't fit perfectly, but probably better
to put it here than building i.MX7 specific fabric code around the DW
PCIe driver.

[...]
> >> +             regmap_update_bits(imx7src->regmap,
> >> +                                SRC_PCIEPHY_RCR,
> >> +                                SRC_PCIEPHY_RCR_PCIEPHY_BTN,
> >> +                                SRC_PCIEPHY_RCR_PCIEPHY_BTN);
> >
> > What is the PCIE PHY button, and why does it have to be set with the
> > reset bit?
> 
> I am sorry, but just as above, I wouldn't be able to enlighten you any
> more about the subject. I have no knowledge about the details of G_RST
> and BTN signal (or even if BTN stands for "button") behavior beyond
> the fact that that is how downstream driver performs PCIEPHY reset.

The SRC_PCIEPHY_RCR register documentation in the i.MX 7Dual
Applications Processor Reference Manual v0.1 describes this bit as "PCIE
PHY button".

[...]
> >> +#define IMX7_RESET_PCIE_CTRL_APPS    0
> >> +#define IMX7_RESET_PCIEPHY           1
> >
> > It would expect these to be numbered in the order they appear in the
> > register map, not starting from the end. Could you add all available
> > peripheral resets to this list, in the correct order?
> 
> The numbering is just a consequence of me adding only resets I could
> exercise with my code and numbering then starting from zero. I also
> was hesitant adding more sources since it seemed to me that some of
> less trivial registers in that IP block might be best represented as a
> single reset source doing something more sophisticated that just
> setting a bit under the hood.

Any in particular?

regards
Philipp
Andrey Smirnov Feb. 14, 2017, 8:11 p.m. UTC | #4
On Tue, Feb 14, 2017 at 8:31 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Tue, 2017-02-14 at 07:46 -0800, Andrey Smirnov wrote:
> [...]
>> >> +enum imx7_src_registers {
>> >> +     SRC_PCIEPHY_RCR = 0x002c,
>> >> +     SRC_PCIEPHY_RCR_PCIEPHY_G_RST     = BIT(1),
>> >
>> > What about the others resets? There's at least HSICPHY, MIPIPHY and
>> > USBOPHY registers before the PCIEPHY register.
>>
>> I don't really have any code using anything but PCI reset related
>> functionality, so I can't test any of the resets you mention. In light
>> of that I didn't want to add any of the definitions that are not going
>> to be used anywhere in the code.
>
> Since the device tree bindings are supposed to be descriptive of the
> hardware, not of the development history of the linux driver, I'd like
> the reset controls to be in a sensible order. From a quick scan of the
> chapter in the reference manual, this is the complete list of resets:
>
> A7_CORE_POR_RESET0
> A7_CORE_POR_RESET1
> A7_CORE_RESET0
> A7_CORE_RESET1
> A7_DBG_RESET0
> A7_DBG_RESET1
> A7_ETM_RESET0
> A7_ETM_RESET1
> A7_SOC_DBG_RESET
> A7_L2RESET
> SW_M4C_RST
> SW_M4P_RST
> EIM_RST
> HSICPHY_PORT_RST
> HSIC_PHY_POR (or Reserved, the manual is conflicted about this)
> USBPHY1_POR
> USBPHY1_PORT_RST
> USBPHY2_POR
> USBPHY2_PORT_RST
> MIPI_PHY_MRST
> MIPI_PHY_SRST
> PCIEPHY_G_RST
> PCIEPHY_BTN
> PCIEPHY_PERST
> PCIE_CTRL_APPS_RST/EN
>
> Arguably, the A7 resets should not be handled by the peripheral reset
> controller, but at least for the others I see no reason not to leave
> space for them in the index table.

If for some bizarre reason one was to run Linux on M4 and use A7 as
applications processor, resetting A7 might be useful. But that's a
very unlikely use-case.

> In fact, since unused reset controls
> don't use space, why not number them all?

IMHO because it is unused code and because those numbers constitute an
interface which once set will be hard to change if need be.

But that's not that important and I don't feel particularly strong
about that point, so I'll add those sources in v3.

Do you insist that I split what I call IMX7_RESET_PCIEPHY into
PCIEPHY_G_RST and PCIEPHY_BTN or can I keep it as a single logical
reset?

>
>> >
>> >> +     SRC_PCIEPHY_RCR_PCIEPHY_BTN       = BIT(2),
>> >> +     SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN = BIT(6),
>> >
>> > Are those really resets? At least the PCIE_CTRL_APPS_EN has a bit called
>> > PCIE_CTRL_APPS_RST right next to it, so this warrants some explanation.
>>
>> Public documentation for that aspect of i.MX7 is nonexistent and,
>> unfortunately, that is my only source of information. Given that, I
>> can't really tell you what the difference between PCIE_CTRL_APPS_EN
>> and PCIE_CTRL_APPS_RST besides that the former is what downstream PCIe
>> driver uses to inhibit LTSSM and the latter is not referenced or used
>> by any code (as far as I am aware).
>
> Ok. That's unfortunate, but nothing we can do about that.
> After discussing with Lucas, I've come to the belief that APPS_EN stops
> the LTSSM, whereas APPS_RST might reset it into the initial state. Since
> the latter doesn't exist at all on i.MX6, and we can obviously live
> without it, I'm fine with just calling the enable bit an active low
> reset to hold it in place. It doesn't fit perfectly, but probably better
> to put it here than building i.MX7 specific fabric code around the DW
> PCIe driver.
>
> [...]
>> >> +             regmap_update_bits(imx7src->regmap,
>> >> +                                SRC_PCIEPHY_RCR,
>> >> +                                SRC_PCIEPHY_RCR_PCIEPHY_BTN,
>> >> +                                SRC_PCIEPHY_RCR_PCIEPHY_BTN);
>> >
>> > What is the PCIE PHY button, and why does it have to be set with the
>> > reset bit?
>>
>> I am sorry, but just as above, I wouldn't be able to enlighten you any
>> more about the subject. I have no knowledge about the details of G_RST
>> and BTN signal (or even if BTN stands for "button") behavior beyond
>> the fact that that is how downstream driver performs PCIEPHY reset.
>
> The SRC_PCIEPHY_RCR register documentation in the i.MX 7Dual
> Applications Processor Reference Manual v0.1 describes this bit as "PCIE
> PHY button".

Oh, yes, that's right. Missed that the datasheet spells "button" explicitly.

>
> [...]
>> >> +#define IMX7_RESET_PCIE_CTRL_APPS    0
>> >> +#define IMX7_RESET_PCIEPHY           1
>> >
>> > It would expect these to be numbered in the order they appear in the
>> > register map, not starting from the end. Could you add all available
>> > peripheral resets to this list, in the correct order?
>>
>> The numbering is just a consequence of me adding only resets I could
>> exercise with my code and numbering then starting from zero. I also
>> was hesitant adding more sources since it seemed to me that some of
>> less trivial registers in that IP block might be best represented as a
>> single reset source doing something more sophisticated that just
>> setting a bit under the hood.
>
> Any in particular?

USBPHY1/2 and maybe MIPI resets? But that's no more than a gut feeling.

Thanks,
Andrey Smirnov
Philipp Zabel Feb. 15, 2017, 9:15 a.m. UTC | #5
On Tue, 2017-02-14 at 12:11 -0800, Andrey Smirnov wrote:
[...]
> > Arguably, the A7 resets should not be handled by the peripheral reset
> > controller, but at least for the others I see no reason not to leave
> > space for them in the index table.
> 
> If for some bizarre reason one was to run Linux on M4 and use A7 as
> applications processor, resetting A7 might be useful. But that's a
> very unlikely use-case.
>
> > In fact, since unused reset controls
> > don't use space, why not number them all?
> 
> IMHO because it is unused code and because those numbers constitute an
> interface which once set will be hard to change if need be.
> 
> But that's not that important and I don't feel particularly strong
> about that point, so I'll add those sources in v3.

Thanks.

> Do you insist that I split what I call IMX7_RESET_PCIEPHY into
> PCIEPHY_G_RST and PCIEPHY_BTN or can I keep it as a single logical
> reset?

No, I say keep it as is. For now I'll assume this is not a reset, but
some other interface signal that just happens to be routed out to the
SRC and just happens to be toggled around the same time in the enable
sequence.

[...]
> >> >> +#define IMX7_RESET_PCIE_CTRL_APPS    0
> >> >> +#define IMX7_RESET_PCIEPHY           1
> >> >
> >> > It would expect these to be numbered in the order they appear in the
> >> > register map, not starting from the end. Could you add all available
> >> > peripheral resets to this list, in the correct order?
> >>
> >> The numbering is just a consequence of me adding only resets I could
> >> exercise with my code and numbering then starting from zero. I also
> >> was hesitant adding more sources since it seemed to me that some of
> >> less trivial registers in that IP block might be best represented as a
> >> single reset source doing something more sophisticated that just
> >> setting a bit under the hood.
> >
> > Any in particular?
> 
> USBPHY1/2 and maybe MIPI resets? But that's no more than a gut feeling.

Is there any downstream code that already handles these resets? At least
for the USB PHYs I'd expect there has to be something we could look at.

regards
Philipp
Andrey Smirnov Feb. 16, 2017, 4:58 a.m. UTC | #6
On Wed, Feb 15, 2017 at 1:15 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Tue, 2017-02-14 at 12:11 -0800, Andrey Smirnov wrote:
> [...]
>> > Arguably, the A7 resets should not be handled by the peripheral reset
>> > controller, but at least for the others I see no reason not to leave
>> > space for them in the index table.
>>
>> If for some bizarre reason one was to run Linux on M4 and use A7 as
>> applications processor, resetting A7 might be useful. But that's a
>> very unlikely use-case.
>>
>> > In fact, since unused reset controls
>> > don't use space, why not number them all?
>>
>> IMHO because it is unused code and because those numbers constitute an
>> interface which once set will be hard to change if need be.
>>
>> But that's not that important and I don't feel particularly strong
>> about that point, so I'll add those sources in v3.
>
> Thanks.
>
>> Do you insist that I split what I call IMX7_RESET_PCIEPHY into
>> PCIEPHY_G_RST and PCIEPHY_BTN or can I keep it as a single logical
>> reset?
>
> No, I say keep it as is. For now I'll assume this is not a reset, but
> some other interface signal that just happens to be routed out to the
> SRC and just happens to be toggled around the same time in the enable
> sequence.
>
> [...]
>> >> >> +#define IMX7_RESET_PCIE_CTRL_APPS    0
>> >> >> +#define IMX7_RESET_PCIEPHY           1
>> >> >
>> >> > It would expect these to be numbered in the order they appear in the
>> >> > register map, not starting from the end. Could you add all available
>> >> > peripheral resets to this list, in the correct order?
>> >>
>> >> The numbering is just a consequence of me adding only resets I could
>> >> exercise with my code and numbering then starting from zero. I also
>> >> was hesitant adding more sources since it seemed to me that some of
>> >> less trivial registers in that IP block might be best represented as a
>> >> single reset source doing something more sophisticated that just
>> >> setting a bit under the hood.
>> >
>> > Any in particular?
>>
>> USBPHY1/2 and maybe MIPI resets? But that's no more than a gut feeling.
>
> Is there any downstream code that already handles these resets? At least
> for the USB PHYs I'd expect there has to be something we could look at.

Just went and double checked. Of the three that I mentioned, couldn't
find anything besides MIPI and for that only one of the signals was
being used, nothing fancy. I think I'll just expose all of the signals
as discussed above and not worry about it in v3.

Thanks,
Andrey Smirnov
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
new file mode 100644
index 0000000..c38f7f0
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt
@@ -0,0 +1,47 @@ 
+Freescale i.MX7 System Reset Controller
+======================================
+
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+Required properties:
+- compatible: Should be "fsl,imx7-src", "syscon"
+- reg: should be register base and length as documented in the
+  datasheet
+- interrupts: Should contain SRC interrupt
+- #reset-cells: 1, see below
+
+example:
+
+src: reset-controller@30390000 {
+     compatible = "fsl,imx7d-src", "syscon";
+     reg = <0x30390000 0x2000>;
+     interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
+     #reset-cells = <1>;
+};
+
+
+Specifying reset lines connected to IP modules
+==============================================
+
+The system reset controller can be used to reset various set of
+peripherals. Device nodes that need access to reset lines should
+specify them as a reset phandle in their corresponding node as
+specified in reset.txt.
+
+Example:
+
+	pcie: pcie@33800000 {
+
+		...
+
+		resets = <&src IMX7_RESET_PCIEPHY>,
+			 <&src IMX7_RESET_PCIE_CTRL_APPS>;
+		reset-names = "pciephy", "apps";
+
+		...
+        };
+
+
+For list of all valid reset indicies see
+<dt-bindings/reset/imx7-reset.h>
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 172dc96..664a88b 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -92,6 +92,14 @@  config RESET_ZYNQ
 	help
 	  This enables the reset controller driver for Xilinx Zynq SoCs.
 
+config RESET_IMX7
+	bool "i.MX7 Reset Driver"
+	depends on SOC_IMX7D || COMPILE_TEST
+	select MFD_SYSCON
+	default SOC_IMX7D
+	help
+	  This enables the reset controller driver for i.MX7 SoCs.
+
 source "drivers/reset/sti/Kconfig"
 source "drivers/reset/hisilicon/Kconfig"
 source "drivers/reset/tegra/Kconfig"
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 13b346e..417c9d0 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -14,3 +14,4 @@  obj-$(CONFIG_RESET_SUNXI) += reset-sunxi.o
 obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o
 obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
 obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
+obj-$(CONFIG_RESET_IMX7) += reset-imx7.o
diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
new file mode 100644
index 0000000..6d90e57
--- /dev/null
+++ b/drivers/reset/reset-imx7.c
@@ -0,0 +1,149 @@ 
+/*
+ * Copyright (c) 2017, Impinj, Inc.
+ *
+ * i.MX7 System Reset Controller (SRC) driver
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.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; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/init.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+#include <dt-bindings/reset/imx7-reset.h>
+
+struct imx7_src {
+	struct reset_controller_dev rcdev;
+	struct device *dev;
+	struct regmap *regmap;
+};
+
+enum imx7_src_registers {
+	SRC_PCIEPHY_RCR = 0x002c,
+	SRC_PCIEPHY_RCR_PCIEPHY_G_RST     = BIT(1),
+	SRC_PCIEPHY_RCR_PCIEPHY_BTN       = BIT(2),
+	SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN = BIT(6),
+};
+
+static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct imx7_src, rcdev);
+}
+
+static int imx7_reset_assert(struct reset_controller_dev *rcdev,
+			     unsigned long id)
+{
+	struct imx7_src *imx7src = to_imx7_src(rcdev);
+
+	switch (id) {
+	case IMX7_RESET_PCIE_CTRL_APPS:
+		regmap_update_bits(imx7src->regmap,
+				   SRC_PCIEPHY_RCR,
+				   SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN, 0);
+		return 0;
+
+	case IMX7_RESET_PCIEPHY:
+		regmap_update_bits(imx7src->regmap,
+				   SRC_PCIEPHY_RCR,
+				   SRC_PCIEPHY_RCR_PCIEPHY_G_RST,
+				   SRC_PCIEPHY_RCR_PCIEPHY_G_RST);
+		regmap_update_bits(imx7src->regmap,
+				   SRC_PCIEPHY_RCR,
+				   SRC_PCIEPHY_RCR_PCIEPHY_BTN,
+				   SRC_PCIEPHY_RCR_PCIEPHY_BTN);
+		return 0;
+	default:
+		dev_err(imx7src->dev, "Unknown reset ID %lu\n", id);
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int imx7_reset_deassert(struct reset_controller_dev *rcdev,
+			       unsigned long id)
+{
+	struct imx7_src *imx7src = to_imx7_src(rcdev);
+
+	switch (id) {
+	case IMX7_RESET_PCIE_CTRL_APPS:
+		regmap_update_bits(imx7src->regmap,
+				   SRC_PCIEPHY_RCR,
+				   SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN,
+				   SRC_PCIEPHY_RCR_PCIE_CTRL_APPS_EN);
+		return 0;
+
+	case IMX7_RESET_PCIEPHY:
+		/* wait for more than 10us to release phy g_rst and btnrst */
+		udelay(10);
+		regmap_update_bits(imx7src->regmap,
+				   SRC_PCIEPHY_RCR,
+				   SRC_PCIEPHY_RCR_PCIEPHY_G_RST, 0);
+		regmap_update_bits(imx7src->regmap,
+				   SRC_PCIEPHY_RCR,
+				   SRC_PCIEPHY_RCR_PCIEPHY_BTN, 0);
+		return 0;
+	default:
+		dev_err(imx7src->dev, "Unknown reset ID %lu\n", id);
+		break;
+	};
+
+	return -EINVAL;
+}
+
+static const struct reset_control_ops imx7_reset_ops = {
+	.assert		= imx7_reset_assert,
+	.deassert	= imx7_reset_deassert,
+};
+
+static int imx7_reset_probe(struct platform_device *pdev)
+{
+	struct imx7_src *imx7src;
+	struct device *dev = &pdev->dev;
+
+	imx7src = devm_kzalloc(dev, sizeof(*imx7src), GFP_KERNEL);
+	if (!imx7src)
+		return -ENOMEM;
+
+	imx7src->dev = dev;
+	imx7src->regmap = syscon_node_to_regmap(dev->of_node);
+	if (IS_ERR(imx7src->regmap)) {
+		dev_err(dev, "Unable to get imx7-src regmap");
+		return PTR_ERR(imx7src->regmap);
+	}
+
+	imx7src->rcdev.owner     = THIS_MODULE;
+	imx7src->rcdev.nr_resets = IMX7_RESET_NUM;
+	imx7src->rcdev.ops       = &imx7_reset_ops;
+	imx7src->rcdev.of_node   = dev->of_node;
+
+	return devm_reset_controller_register(dev, &imx7src->rcdev);
+}
+
+static const struct of_device_id imx7_reset_dt_ids[] = {
+	{ .compatible = "fsl,imx7d-src", },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver imx7_reset_driver = {
+	.probe	= imx7_reset_probe,
+	.driver = {
+		.name		= KBUILD_MODNAME,
+		.of_match_table	= imx7_reset_dt_ids,
+	},
+};
+builtin_platform_driver(imx7_reset_driver);
diff --git a/include/dt-bindings/reset/imx7-reset.h b/include/dt-bindings/reset/imx7-reset.h
new file mode 100644
index 0000000..e20f2db
--- /dev/null
+++ b/include/dt-bindings/reset/imx7-reset.h
@@ -0,0 +1,28 @@ 
+/*
+ * Copyright (C) 2017 Impinj, Inc.
+ *
+ * Author: Andrey Smirnov <andrew.smirnov@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef DT_BINDING_RESET_IMX7_H
+#define DT_BINDING_RESET_IMX7_H
+
+#define IMX7_RESET_PCIE_CTRL_APPS	0
+#define IMX7_RESET_PCIEPHY		1
+
+#define IMX7_RESET_NUM			2
+
+#endif
+