From: Andrey Smirnov <andrew.smirnov@gmail.com> To: Philipp Zabel <p.zabel@pengutronix.de> Cc: linux-kernel <linux-kernel@vger.kernel.org>, Fabio Estevam <fabio.estevam@nxp.com>, Chris Healy <cphealy@gmail.com>, Lucas Stach <l.stach@pengutronix.de>, Leonard Crestez <leonard.crestez@nxp.com>, Dong Aisheng <aisheng.dong@nxp.com>, Richard Zhu <hongxing.zhu@nxp.com>, linux-imx@nxp.com, linux-arm-kernel <linux-arm-kernel@lists.infradead.org> Subject: Re: [PATCH 1/1] reset: imx7: Add support for i.MX8MQ Date: Mon, 26 Nov 2018 07:59:37 -0800 [thread overview] Message-ID: <CAHQ1cqFEKHR3VW9gfaZcp-BJeh5PTM85F1kPF0am0rmaW9HiRQ@mail.gmail.com> (raw) In-Reply-To: <1542637950.5139.12.camel@pengutronix.de> On Mon, Nov 19, 2018 at 6:32 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Andrey, > > thank you for the patch. > > In general, sharing the lookup table with i.MX7 is fine iff it is a > strict superset. But I don't think that is the case (see below). > Even so, this will change if there ever is another i.MX7 or i.MX8M > variant that is also a superset of i.MX7, but not a superset of > i.MX8M. Also, just listing all resets in order of appearance would make > review a bit easier. > > I don't like increasing IMX7_RESET_NUM though, i.MX8M should have its > own nr_resets. > > On Sat, 2018-11-17 at 10:11 -0800, Andrey Smirnov wrote: > > SRC IP block used in i.MX8MQ is a superset of what is found in i.MX7D, > > Is this true, though? > > i.MX7 has SRC_ERCR at offset 0x14 and HSICPHY_RCR at offset 0x1c. > According to documentation, i.MX8M is missing those at least. > OK, I'll convert the patch to use separate LUTs in v2. > > so add all of the definitions necessary to support both. > > > > Cc: p.zabel@pengutronix.de > > Cc: Fabio Estevam <fabio.estevam@nxp.com> > > Cc: cphealy@gmail.com > > Cc: l.stach@pengutronix.de > > Cc: Leonard Crestez <leonard.crestez@nxp.com> > > Cc: "A.s. Dong" <aisheng.dong@nxp.com> > > Cc: Richard Zhu <hongxing.zhu@nxp.com> > > Cc: linux-imx@nxp.com > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: linux-kernel@vger.kernel.org > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > --- > > drivers/reset/Kconfig | 2 +- > > drivers/reset/reset-imx7.c | 66 ++++++++++++++++++++++++- > > include/dt-bindings/reset/imx7-reset.h | 15 +++++- > > include/dt-bindings/reset/imx8m-reset.h | 47 ++++++++++++++++++ > > 4 files changed, 127 insertions(+), 3 deletions(-) > > create mode 100644 include/dt-bindings/reset/imx8m-reset.h > > > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > > index c21da9fe51ec..4909aab7401b 100644 > > --- a/drivers/reset/Kconfig > > +++ b/drivers/reset/Kconfig > > @@ -50,7 +50,7 @@ config RESET_HSDK > > config RESET_IMX7 > > bool "i.MX7 Reset Driver" if COMPILE_TEST > > depends on HAS_IOMEM > > - default SOC_IMX7D > > + default SOC_IMX7D || SOC_IMX8MQ > > select MFD_SYSCON > > help > > This enables the reset controller driver for i.MX7 SoCs. > > diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c > > index 77911fa8f31d..dffad618f805 100644 > > --- a/drivers/reset/reset-imx7.c > > +++ b/drivers/reset/reset-imx7.c > > @@ -21,14 +21,16 @@ > > #include <linux/reset-controller.h> > > #include <linux/regmap.h> > > #include <dt-bindings/reset/imx7-reset.h> > > +#include <dt-bindings/reset/imx8m-reset.h> > > > > struct imx7_src { > > struct reset_controller_dev rcdev; > > struct regmap *regmap; > > }; > > > > -enum imx7_src_registers { > > +enum imx_src_registers { > > SRC_A7RCR0 = 0x0004, > > + SRC_A53RCR0 = 0x0004, > > SRC_M4RCR = 0x000c, > > SRC_ERCR = 0x0014, > > SRC_HSICPHY_RCR = 0x001c, > > @@ -36,7 +38,9 @@ enum imx7_src_registers { > > SRC_USBOPHY2_RCR = 0x0024, > > SRC_MIPIPHY_RCR = 0x0028, > > SRC_PCIEPHY_RCR = 0x002c, > > These aren't complete. I see at least SRC_HDMI_RCR, SRC_DISP_RCR, > SRC_GPU_RCR, and SRC_VPU_RCR missing. > > > + SRC_PCIE2PHY_RCR = 0x0048, > > And SRC_MIPIPHY1_RCR, SRC_MIPIPHY2_RCR. Please check the reference > manual and complete the list of registers that contain resets. > > > SRC_DDRC_RCR = 0x1000, > > + > > Whitespace. > > > }; > > > > struct imx7_src_signal { > > @@ -67,11 +71,67 @@ static const struct imx7_src_signal imx7_src_signals[IMX7_RESET_NUM] = { > > [IMX7_RESET_PCIEPHY] = { SRC_PCIEPHY_RCR, BIT(2) | BIT(1) }, > > [IMX7_RESET_PCIEPHY_PERST] = { SRC_PCIEPHY_RCR, BIT(3) }, > > [IMX7_RESET_PCIE_CTRL_APPS_EN] = { SRC_PCIEPHY_RCR, BIT(6) }, > > + [IMX7_RESET_PCIE_CTRL_APPS_CLK_REQ] = { SRC_PCIEPHY_RCR, BIT(4) }, > > Now I feel like CTRL_APPS_EN shouldn't have been added here either. But > while there I wasn't really sure about whether that is actually a valid > reset, I'm pretty sure that this one isn't. > I think that i.MX8M either needs a clock driver to control this bit and > the corresponding IOMUXC GPR bit to enable the PCIe refclk, or the PCIe > driver has to access this register via syscon. I really don't want to > see a clock enable signal described in the device tree as a reset. > OK, will drop in v2. > > [IMX7_RESET_PCIE_CTRL_APPS_TURNOFF] = { SRC_PCIEPHY_RCR, BIT(11) }, > > [IMX7_RESET_DDRC_PRST] = { SRC_DDRC_RCR, BIT(0) }, > > [IMX7_RESET_DDRC_CORE_RST] = { SRC_DDRC_RCR, BIT(1) }, > > + > > + [IMX8M_RESET_A53_CORE_POR_RESET2] = { SRC_A53RCR0, BIT(2) }, > > + [IMX8M_RESET_A53_CORE_POR_RESET3] = { SRC_A53RCR0, BIT(3) }, > > + [IMX8M_RESET_A53_CORE_RESET2] = { SRC_A53RCR0, BIT(6) }, > > + [IMX8M_RESET_A53_CORE_RESET3] = { SRC_A53RCR0, BIT(7) }, > > Missing IMX8M_RESET_A53_DBG_RESET2,3. > > > + [IMX8M_RESET_A53_ETM_RESET2] = { SRC_A53RCR0, BIT(14) }, > > + [IMX8M_RESET_A53_ETM_RESET3] = { SRC_A53RCR0, BIT(15) }, > > Missing some reset registers here. > > > + [IMX8M_RESET_PCIE2PHY] = { SRC_PCIEPHY_RCR, BIT(2) | BIT(1) }, > > + [IMX8M_RESET_PCIE2PHY_PERST] = { SRC_PCIE2PHY_RCR, BIT(3) }, > > + [IMX8M_RESET_PCIE2_CTRL_APPS_EN] = { SRC_PCIE2PHY_RCR, BIT(6) }, > > + [IMX8M_RESET_PCIE2_CTRL_APPS_CLK_REQ] = { SRC_PCIE2PHY_RCR, BIT(4) }, > > See above. > > > + [IMX8M_RESET_PCIE2_CTRL_APPS_TURNOFF] = { SRC_PCIE2PHY_RCR, BIT(11) }, > > Missing some reset registers here. > > > }; > > > > +static inline void imx7_src_check_definitions(void) > > +{ > > + BUILD_BUG_ON(IMX8M_RESET_A53_CORE_POR_RESET0 != > > + IMX7_RESET_A7_CORE_POR_RESET0); > > + BUILD_BUG_ON(IMX8M_RESET_A53_CORE_POR_RESET1 != > > + IMX7_RESET_A7_CORE_POR_RESET1); > > + BUILD_BUG_ON(IMX8M_RESET_A53_CORE_RESET0 != > > + IMX7_RESET_A7_CORE_RESET0); > > + BUILD_BUG_ON(IMX8M_RESET_A53_CORE_RESET1 != > > + IMX7_RESET_A7_CORE_RESET1); > > + BUILD_BUG_ON(IMX8M_RESET_A53_DBG_RESET0 != > > + IMX7_RESET_A7_DBG_RESET0); > > + BUILD_BUG_ON(IMX8M_RESET_A53_DBG_RESET1 != > > + IMX7_RESET_A7_DBG_RESET1); > > + BUILD_BUG_ON(IMX8M_RESET_A53_ETM_RESET0 != > > + IMX7_RESET_A7_ETM_RESET0); > > + BUILD_BUG_ON(IMX8M_RESET_A53_ETM_RESET1 != > > + IMX7_RESET_A7_ETM_RESET1); > > + BUILD_BUG_ON(IMX8M_RESET_A53_SOC_DBG_RESET != > > + IMX7_RESET_A7_SOC_DBG_RESET); > > + BUILD_BUG_ON(IMX8M_RESET_A53_L2RESET != IMX7_RESET_A7_L2RESET); > > + BUILD_BUG_ON(IMX8M_RESET_SW_M4C_RST != IMX7_RESET_SW_M4C_RST); > > + BUILD_BUG_ON(IMX8M_RESET_SW_M4P_RST != IMX7_RESET_SW_M4P_RST); > > + BUILD_BUG_ON(IMX8M_RESET_EIM_RST != IMX7_RESET_EIM_RST); > > + BUILD_BUG_ON(IMX8M_RESET_HSICPHY_PORT_RST != > > + IMX7_RESET_HSICPHY_PORT_RST); > > + BUILD_BUG_ON(IMX8M_RESET_USBPHY1_POR != IMX7_RESET_USBPHY1_POR); > > + BUILD_BUG_ON(IMX8M_RESET_USBPHY1_PORT_RST != > > + IMX7_RESET_USBPHY1_PORT_RST); > > + BUILD_BUG_ON(IMX8M_RESET_USBPHY2_POR != IMX7_RESET_USBPHY2_POR); > > + BUILD_BUG_ON(IMX8M_RESET_USBPHY2_PORT_RST != > > + IMX7_RESET_USBPHY2_PORT_RST); > > + BUILD_BUG_ON(IMX8M_RESET_MIPI_PHY_MRST != IMX7_RESET_MIPI_PHY_MRST); > > + BUILD_BUG_ON(IMX8M_RESET_MIPI_PHY_SRST != IMX7_RESET_MIPI_PHY_SRST); > > + BUILD_BUG_ON(IMX8M_RESET_PCIEPHY != IMX7_RESET_PCIEPHY); > > + BUILD_BUG_ON(IMX8M_RESET_PCIE_CTRL_APPS_EN != > > + IMX7_RESET_PCIE_CTRL_APPS_EN); > > + BUILD_BUG_ON(IMX8M_RESET_PCIE_CTRL_APPS_TURNOFF != > > + IMX7_RESET_PCIE_CTRL_APPS_TURNOFF); > > + BUILD_BUG_ON(IMX8M_RESET_DDRC_PRST != IMX7_RESET_DDRC_PRST); > > + BUILD_BUG_ON(IMX8M_RESET_DDRC_CORE_RST != IMX7_RESET_DDRC_CORE_RST); > > +} > > This won't be necessary with a separate lookup table. > > > static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev) > > { > > return container_of(rcdev, struct imx7_src, rcdev); > > @@ -85,6 +145,7 @@ static int imx7_reset_set(struct reset_controller_dev *rcdev, > > unsigned int value = assert ? signal->bit : 0; > > > > switch (id) { > > + case IMX8M_RESET_PCIE2PHY: /* FALLTHROUGH */ > > I'd prefer lowercase /* fallthrough */. > > > case IMX7_RESET_PCIEPHY: > > /* > > * wait for more than 10us to release phy g_rst and > > @@ -94,6 +155,7 @@ static int imx7_reset_set(struct reset_controller_dev *rcdev, > > udelay(10); > > break; > > > > + case IMX8M_RESET_PCIE2_CTRL_APPS_EN: /* FALLTHROUGH */ > > And here. > > > case IMX7_RESET_PCIE_CTRL_APPS_EN: > > value = (assert) ? 0 : signal->bit; > > break; > > @@ -126,6 +188,8 @@ static int imx7_reset_probe(struct platform_device *pdev) > > struct device *dev = &pdev->dev; > > struct regmap_config config = { .name = "src" }; > > > > + imx7_src_check_definitions(); > > + > > imx7src = devm_kzalloc(dev, sizeof(*imx7src), GFP_KERNEL); > > if (!imx7src) > > return -ENOMEM; > > diff --git a/include/dt-bindings/reset/imx7-reset.h b/include/dt-bindings/reset/imx7-reset.h > > index 31b3f87dde9a..8fefd694d481 100644 > > --- a/include/dt-bindings/reset/imx7-reset.h > > +++ b/include/dt-bindings/reset/imx7-reset.h > > @@ -57,8 +57,21 @@ > > #define IMX7_RESET_DDRC_CORE_RST 24 > > > > #define IMX7_RESET_PCIE_CTRL_APPS_TURNOFF 25 > > +#define IMX7_RESET_PCIE_CTRL_APPS_CLK_REQ 26 > > See above. > > > -#define IMX7_RESET_NUM 26 > > Please leave this at 26. > > > +#define IMX8M_RESET_A53_CORE_POR_RESET2 27 > > +#define IMX8M_RESET_A53_CORE_POR_RESET3 28 > > +#define IMX8M_RESET_A53_CORE_RESET2 29 > > +#define IMX8M_RESET_A53_CORE_RESET3 30 > > +#define IMX8M_RESET_A53_ETM_RESET2 31 > > +#define IMX8M_RESET_A53_ETM_RESET3 32 > > +#define IMX8M_RESET_PCIE2PHY 33 > > +#define IMX8M_RESET_PCIE2PHY_PERST 34 > > +#define IMX8M_RESET_PCIE2_CTRL_APPS_EN 35 > > +#define IMX8M_RESET_PCIE2_CTRL_APPS_CLK_REQ 36 > > See above. > > > +#define IMX8M_RESET_PCIE2_CTRL_APPS_TURNOFF 37 > > Move these into imx8m-reset.h > > > + > > +#define IMX7_RESET_NUM 38 > > And add a separate IMX8M_RESET_NUM. > > > > > #endif > > > > diff --git a/include/dt-bindings/reset/imx8m-reset.h b/include/dt-bindings/reset/imx8m-reset.h > > new file mode 100644 > > index 000000000000..8fa840354723 > > --- /dev/null > > +++ b/include/dt-bindings/reset/imx8m-reset.h > > @@ -0,0 +1,47 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2018 Zodiac Inflight Innovations > > + * > > + * Author: Andrey Smirnov <andrew.smirnov@gmail.com> > > + */ > > + > > +#ifndef DT_BINDING_RESET_IMX8M_H > > +#define DT_BINDING_RESET_IMX8M_H > > + > > +#include "imx7-reset.h" > > + > > +#define IMX8M_RESET_A53_CORE_POR_RESET0 IMX7_RESET_A7_CORE_POR_RESET0 > > +#define IMX8M_RESET_A53_CORE_POR_RESET1 IMX7_RESET_A7_CORE_POR_RESET1 > > + > > +#define IMX8M_RESET_A53_CORE_RESET0 IMX7_RESET_A7_CORE_RESET0 > > +#define IMX8M_RESET_A53_CORE_RESET1 IMX7_RESET_A7_CORE_RESET1 > > + > > +#define IMX8M_RESET_A53_DBG_RESET0 IMX7_RESET_A7_DBG_RESET0 > > +#define IMX8M_RESET_A53_DBG_RESET1 IMX7_RESET_A7_DBG_RESET1 > > + > > +#define IMX8M_RESET_A53_ETM_RESET0 IMX7_RESET_A7_ETM_RESET0 > > +#define IMX8M_RESET_A53_ETM_RESET1 IMX7_RESET_A7_ETM_RESET1 > > + > > +#define IMX8M_RESET_A53_SOC_DBG_RESET IMX7_RESET_A7_SOC_DBG_RESET > > +#define IMX8M_RESET_A53_L2RESET IMX7_RESET_A7_L2RESET > > +#define IMX8M_RESET_SW_M4C_RST IMX7_RESET_SW_M4C_RST > > +#define IMX8M_RESET_SW_M4P_RST IMX7_RESET_SW_M4P_RST > > > +#define IMX8M_RESET_EIM_RST IMX7_RESET_EIM_RST > > +#define IMX8M_RESET_HSICPHY_PORT_RST IMX7_RESET_HSICPHY_PORT_RST > > These two don't exist according to the reference manual. > > > +#define IMX8M_RESET_USBPHY1_POR IMX7_RESET_USBPHY1_POR > > +#define IMX8M_RESET_USBPHY1_PORT_RST IMX7_RESET_USBPHY1_PORT_RST > > These two don't seem to exist either. I only see a single OTG1_PHY_RESET > bit in that register. > > > +#define IMX8M_RESET_USBPHY2_POR IMX7_RESET_USBPHY2_POR > > +#define IMX8M_RESET_USBPHY2_PORT_RST IMX7_RESET_USBPHY2_PORT_RST > > Same here. > > > +#define IMX8M_RESET_MIPI_PHY_MRST IMX7_RESET_MIPI_PHY_MRST > > +#define IMX8M_RESET_MIPI_PHY_SRST IMX7_RESET_MIPI_PHY_SRST > > These don't exist either. The MIPIPHY_RCR register instead has 5 > different _RESET_N bits. > OK, I'll incorporate all of the feedback above in v2 as well. Thanks, Andrey Smirnov
WARNING: multiple messages have this Message-ID (diff)
From: andrew.smirnov@gmail.com (Andrey Smirnov) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 1/1] reset: imx7: Add support for i.MX8MQ Date: Mon, 26 Nov 2018 07:59:37 -0800 [thread overview] Message-ID: <CAHQ1cqFEKHR3VW9gfaZcp-BJeh5PTM85F1kPF0am0rmaW9HiRQ@mail.gmail.com> (raw) In-Reply-To: <1542637950.5139.12.camel@pengutronix.de> On Mon, Nov 19, 2018 at 6:32 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Andrey, > > thank you for the patch. > > In general, sharing the lookup table with i.MX7 is fine iff it is a > strict superset. But I don't think that is the case (see below). > Even so, this will change if there ever is another i.MX7 or i.MX8M > variant that is also a superset of i.MX7, but not a superset of > i.MX8M. Also, just listing all resets in order of appearance would make > review a bit easier. > > I don't like increasing IMX7_RESET_NUM though, i.MX8M should have its > own nr_resets. > > On Sat, 2018-11-17 at 10:11 -0800, Andrey Smirnov wrote: > > SRC IP block used in i.MX8MQ is a superset of what is found in i.MX7D, > > Is this true, though? > > i.MX7 has SRC_ERCR at offset 0x14 and HSICPHY_RCR at offset 0x1c. > According to documentation, i.MX8M is missing those at least. > OK, I'll convert the patch to use separate LUTs in v2. > > so add all of the definitions necessary to support both. > > > > Cc: p.zabel at pengutronix.de > > Cc: Fabio Estevam <fabio.estevam@nxp.com> > > Cc: cphealy at gmail.com > > Cc: l.stach at pengutronix.de > > Cc: Leonard Crestez <leonard.crestez@nxp.com> > > Cc: "A.s. Dong" <aisheng.dong@nxp.com> > > Cc: Richard Zhu <hongxing.zhu@nxp.com> > > Cc: linux-imx at nxp.com > > Cc: linux-arm-kernel at lists.infradead.org > > Cc: linux-kernel at vger.kernel.org > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > --- > > drivers/reset/Kconfig | 2 +- > > drivers/reset/reset-imx7.c | 66 ++++++++++++++++++++++++- > > include/dt-bindings/reset/imx7-reset.h | 15 +++++- > > include/dt-bindings/reset/imx8m-reset.h | 47 ++++++++++++++++++ > > 4 files changed, 127 insertions(+), 3 deletions(-) > > create mode 100644 include/dt-bindings/reset/imx8m-reset.h > > > > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig > > index c21da9fe51ec..4909aab7401b 100644 > > --- a/drivers/reset/Kconfig > > +++ b/drivers/reset/Kconfig > > @@ -50,7 +50,7 @@ config RESET_HSDK > > config RESET_IMX7 > > bool "i.MX7 Reset Driver" if COMPILE_TEST > > depends on HAS_IOMEM > > - default SOC_IMX7D > > + default SOC_IMX7D || SOC_IMX8MQ > > select MFD_SYSCON > > help > > This enables the reset controller driver for i.MX7 SoCs. > > diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c > > index 77911fa8f31d..dffad618f805 100644 > > --- a/drivers/reset/reset-imx7.c > > +++ b/drivers/reset/reset-imx7.c > > @@ -21,14 +21,16 @@ > > #include <linux/reset-controller.h> > > #include <linux/regmap.h> > > #include <dt-bindings/reset/imx7-reset.h> > > +#include <dt-bindings/reset/imx8m-reset.h> > > > > struct imx7_src { > > struct reset_controller_dev rcdev; > > struct regmap *regmap; > > }; > > > > -enum imx7_src_registers { > > +enum imx_src_registers { > > SRC_A7RCR0 = 0x0004, > > + SRC_A53RCR0 = 0x0004, > > SRC_M4RCR = 0x000c, > > SRC_ERCR = 0x0014, > > SRC_HSICPHY_RCR = 0x001c, > > @@ -36,7 +38,9 @@ enum imx7_src_registers { > > SRC_USBOPHY2_RCR = 0x0024, > > SRC_MIPIPHY_RCR = 0x0028, > > SRC_PCIEPHY_RCR = 0x002c, > > These aren't complete. I see at least SRC_HDMI_RCR, SRC_DISP_RCR, > SRC_GPU_RCR, and SRC_VPU_RCR missing. > > > + SRC_PCIE2PHY_RCR = 0x0048, > > And SRC_MIPIPHY1_RCR, SRC_MIPIPHY2_RCR. Please check the reference > manual and complete the list of registers that contain resets. > > > SRC_DDRC_RCR = 0x1000, > > + > > Whitespace. > > > }; > > > > struct imx7_src_signal { > > @@ -67,11 +71,67 @@ static const struct imx7_src_signal imx7_src_signals[IMX7_RESET_NUM] = { > > [IMX7_RESET_PCIEPHY] = { SRC_PCIEPHY_RCR, BIT(2) | BIT(1) }, > > [IMX7_RESET_PCIEPHY_PERST] = { SRC_PCIEPHY_RCR, BIT(3) }, > > [IMX7_RESET_PCIE_CTRL_APPS_EN] = { SRC_PCIEPHY_RCR, BIT(6) }, > > + [IMX7_RESET_PCIE_CTRL_APPS_CLK_REQ] = { SRC_PCIEPHY_RCR, BIT(4) }, > > Now I feel like CTRL_APPS_EN shouldn't have been added here either. But > while there I wasn't really sure about whether that is actually a valid > reset, I'm pretty sure that this one isn't. > I think that i.MX8M either needs a clock driver to control this bit and > the corresponding IOMUXC GPR bit to enable the PCIe refclk, or the PCIe > driver has to access this register via syscon. I really don't want to > see a clock enable signal described in the device tree as a reset. > OK, will drop in v2. > > [IMX7_RESET_PCIE_CTRL_APPS_TURNOFF] = { SRC_PCIEPHY_RCR, BIT(11) }, > > [IMX7_RESET_DDRC_PRST] = { SRC_DDRC_RCR, BIT(0) }, > > [IMX7_RESET_DDRC_CORE_RST] = { SRC_DDRC_RCR, BIT(1) }, > > + > > + [IMX8M_RESET_A53_CORE_POR_RESET2] = { SRC_A53RCR0, BIT(2) }, > > + [IMX8M_RESET_A53_CORE_POR_RESET3] = { SRC_A53RCR0, BIT(3) }, > > + [IMX8M_RESET_A53_CORE_RESET2] = { SRC_A53RCR0, BIT(6) }, > > + [IMX8M_RESET_A53_CORE_RESET3] = { SRC_A53RCR0, BIT(7) }, > > Missing IMX8M_RESET_A53_DBG_RESET2,3. > > > + [IMX8M_RESET_A53_ETM_RESET2] = { SRC_A53RCR0, BIT(14) }, > > + [IMX8M_RESET_A53_ETM_RESET3] = { SRC_A53RCR0, BIT(15) }, > > Missing some reset registers here. > > > + [IMX8M_RESET_PCIE2PHY] = { SRC_PCIEPHY_RCR, BIT(2) | BIT(1) }, > > + [IMX8M_RESET_PCIE2PHY_PERST] = { SRC_PCIE2PHY_RCR, BIT(3) }, > > + [IMX8M_RESET_PCIE2_CTRL_APPS_EN] = { SRC_PCIE2PHY_RCR, BIT(6) }, > > + [IMX8M_RESET_PCIE2_CTRL_APPS_CLK_REQ] = { SRC_PCIE2PHY_RCR, BIT(4) }, > > See above. > > > + [IMX8M_RESET_PCIE2_CTRL_APPS_TURNOFF] = { SRC_PCIE2PHY_RCR, BIT(11) }, > > Missing some reset registers here. > > > }; > > > > +static inline void imx7_src_check_definitions(void) > > +{ > > + BUILD_BUG_ON(IMX8M_RESET_A53_CORE_POR_RESET0 != > > + IMX7_RESET_A7_CORE_POR_RESET0); > > + BUILD_BUG_ON(IMX8M_RESET_A53_CORE_POR_RESET1 != > > + IMX7_RESET_A7_CORE_POR_RESET1); > > + BUILD_BUG_ON(IMX8M_RESET_A53_CORE_RESET0 != > > + IMX7_RESET_A7_CORE_RESET0); > > + BUILD_BUG_ON(IMX8M_RESET_A53_CORE_RESET1 != > > + IMX7_RESET_A7_CORE_RESET1); > > + BUILD_BUG_ON(IMX8M_RESET_A53_DBG_RESET0 != > > + IMX7_RESET_A7_DBG_RESET0); > > + BUILD_BUG_ON(IMX8M_RESET_A53_DBG_RESET1 != > > + IMX7_RESET_A7_DBG_RESET1); > > + BUILD_BUG_ON(IMX8M_RESET_A53_ETM_RESET0 != > > + IMX7_RESET_A7_ETM_RESET0); > > + BUILD_BUG_ON(IMX8M_RESET_A53_ETM_RESET1 != > > + IMX7_RESET_A7_ETM_RESET1); > > + BUILD_BUG_ON(IMX8M_RESET_A53_SOC_DBG_RESET != > > + IMX7_RESET_A7_SOC_DBG_RESET); > > + BUILD_BUG_ON(IMX8M_RESET_A53_L2RESET != IMX7_RESET_A7_L2RESET); > > + BUILD_BUG_ON(IMX8M_RESET_SW_M4C_RST != IMX7_RESET_SW_M4C_RST); > > + BUILD_BUG_ON(IMX8M_RESET_SW_M4P_RST != IMX7_RESET_SW_M4P_RST); > > + BUILD_BUG_ON(IMX8M_RESET_EIM_RST != IMX7_RESET_EIM_RST); > > + BUILD_BUG_ON(IMX8M_RESET_HSICPHY_PORT_RST != > > + IMX7_RESET_HSICPHY_PORT_RST); > > + BUILD_BUG_ON(IMX8M_RESET_USBPHY1_POR != IMX7_RESET_USBPHY1_POR); > > + BUILD_BUG_ON(IMX8M_RESET_USBPHY1_PORT_RST != > > + IMX7_RESET_USBPHY1_PORT_RST); > > + BUILD_BUG_ON(IMX8M_RESET_USBPHY2_POR != IMX7_RESET_USBPHY2_POR); > > + BUILD_BUG_ON(IMX8M_RESET_USBPHY2_PORT_RST != > > + IMX7_RESET_USBPHY2_PORT_RST); > > + BUILD_BUG_ON(IMX8M_RESET_MIPI_PHY_MRST != IMX7_RESET_MIPI_PHY_MRST); > > + BUILD_BUG_ON(IMX8M_RESET_MIPI_PHY_SRST != IMX7_RESET_MIPI_PHY_SRST); > > + BUILD_BUG_ON(IMX8M_RESET_PCIEPHY != IMX7_RESET_PCIEPHY); > > + BUILD_BUG_ON(IMX8M_RESET_PCIE_CTRL_APPS_EN != > > + IMX7_RESET_PCIE_CTRL_APPS_EN); > > + BUILD_BUG_ON(IMX8M_RESET_PCIE_CTRL_APPS_TURNOFF != > > + IMX7_RESET_PCIE_CTRL_APPS_TURNOFF); > > + BUILD_BUG_ON(IMX8M_RESET_DDRC_PRST != IMX7_RESET_DDRC_PRST); > > + BUILD_BUG_ON(IMX8M_RESET_DDRC_CORE_RST != IMX7_RESET_DDRC_CORE_RST); > > +} > > This won't be necessary with a separate lookup table. > > > static struct imx7_src *to_imx7_src(struct reset_controller_dev *rcdev) > > { > > return container_of(rcdev, struct imx7_src, rcdev); > > @@ -85,6 +145,7 @@ static int imx7_reset_set(struct reset_controller_dev *rcdev, > > unsigned int value = assert ? signal->bit : 0; > > > > switch (id) { > > + case IMX8M_RESET_PCIE2PHY: /* FALLTHROUGH */ > > I'd prefer lowercase /* fallthrough */. > > > case IMX7_RESET_PCIEPHY: > > /* > > * wait for more than 10us to release phy g_rst and > > @@ -94,6 +155,7 @@ static int imx7_reset_set(struct reset_controller_dev *rcdev, > > udelay(10); > > break; > > > > + case IMX8M_RESET_PCIE2_CTRL_APPS_EN: /* FALLTHROUGH */ > > And here. > > > case IMX7_RESET_PCIE_CTRL_APPS_EN: > > value = (assert) ? 0 : signal->bit; > > break; > > @@ -126,6 +188,8 @@ static int imx7_reset_probe(struct platform_device *pdev) > > struct device *dev = &pdev->dev; > > struct regmap_config config = { .name = "src" }; > > > > + imx7_src_check_definitions(); > > + > > imx7src = devm_kzalloc(dev, sizeof(*imx7src), GFP_KERNEL); > > if (!imx7src) > > return -ENOMEM; > > diff --git a/include/dt-bindings/reset/imx7-reset.h b/include/dt-bindings/reset/imx7-reset.h > > index 31b3f87dde9a..8fefd694d481 100644 > > --- a/include/dt-bindings/reset/imx7-reset.h > > +++ b/include/dt-bindings/reset/imx7-reset.h > > @@ -57,8 +57,21 @@ > > #define IMX7_RESET_DDRC_CORE_RST 24 > > > > #define IMX7_RESET_PCIE_CTRL_APPS_TURNOFF 25 > > +#define IMX7_RESET_PCIE_CTRL_APPS_CLK_REQ 26 > > See above. > > > -#define IMX7_RESET_NUM 26 > > Please leave this at 26. > > > +#define IMX8M_RESET_A53_CORE_POR_RESET2 27 > > +#define IMX8M_RESET_A53_CORE_POR_RESET3 28 > > +#define IMX8M_RESET_A53_CORE_RESET2 29 > > +#define IMX8M_RESET_A53_CORE_RESET3 30 > > +#define IMX8M_RESET_A53_ETM_RESET2 31 > > +#define IMX8M_RESET_A53_ETM_RESET3 32 > > +#define IMX8M_RESET_PCIE2PHY 33 > > +#define IMX8M_RESET_PCIE2PHY_PERST 34 > > +#define IMX8M_RESET_PCIE2_CTRL_APPS_EN 35 > > +#define IMX8M_RESET_PCIE2_CTRL_APPS_CLK_REQ 36 > > See above. > > > +#define IMX8M_RESET_PCIE2_CTRL_APPS_TURNOFF 37 > > Move these into imx8m-reset.h > > > + > > +#define IMX7_RESET_NUM 38 > > And add a separate IMX8M_RESET_NUM. > > > > > #endif > > > > diff --git a/include/dt-bindings/reset/imx8m-reset.h b/include/dt-bindings/reset/imx8m-reset.h > > new file mode 100644 > > index 000000000000..8fa840354723 > > --- /dev/null > > +++ b/include/dt-bindings/reset/imx8m-reset.h > > @@ -0,0 +1,47 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2018 Zodiac Inflight Innovations > > + * > > + * Author: Andrey Smirnov <andrew.smirnov@gmail.com> > > + */ > > + > > +#ifndef DT_BINDING_RESET_IMX8M_H > > +#define DT_BINDING_RESET_IMX8M_H > > + > > +#include "imx7-reset.h" > > + > > +#define IMX8M_RESET_A53_CORE_POR_RESET0 IMX7_RESET_A7_CORE_POR_RESET0 > > +#define IMX8M_RESET_A53_CORE_POR_RESET1 IMX7_RESET_A7_CORE_POR_RESET1 > > + > > +#define IMX8M_RESET_A53_CORE_RESET0 IMX7_RESET_A7_CORE_RESET0 > > +#define IMX8M_RESET_A53_CORE_RESET1 IMX7_RESET_A7_CORE_RESET1 > > + > > +#define IMX8M_RESET_A53_DBG_RESET0 IMX7_RESET_A7_DBG_RESET0 > > +#define IMX8M_RESET_A53_DBG_RESET1 IMX7_RESET_A7_DBG_RESET1 > > + > > +#define IMX8M_RESET_A53_ETM_RESET0 IMX7_RESET_A7_ETM_RESET0 > > +#define IMX8M_RESET_A53_ETM_RESET1 IMX7_RESET_A7_ETM_RESET1 > > + > > +#define IMX8M_RESET_A53_SOC_DBG_RESET IMX7_RESET_A7_SOC_DBG_RESET > > +#define IMX8M_RESET_A53_L2RESET IMX7_RESET_A7_L2RESET > > +#define IMX8M_RESET_SW_M4C_RST IMX7_RESET_SW_M4C_RST > > +#define IMX8M_RESET_SW_M4P_RST IMX7_RESET_SW_M4P_RST > > > +#define IMX8M_RESET_EIM_RST IMX7_RESET_EIM_RST > > +#define IMX8M_RESET_HSICPHY_PORT_RST IMX7_RESET_HSICPHY_PORT_RST > > These two don't exist according to the reference manual. > > > +#define IMX8M_RESET_USBPHY1_POR IMX7_RESET_USBPHY1_POR > > +#define IMX8M_RESET_USBPHY1_PORT_RST IMX7_RESET_USBPHY1_PORT_RST > > These two don't seem to exist either. I only see a single OTG1_PHY_RESET > bit in that register. > > > +#define IMX8M_RESET_USBPHY2_POR IMX7_RESET_USBPHY2_POR > > +#define IMX8M_RESET_USBPHY2_PORT_RST IMX7_RESET_USBPHY2_PORT_RST > > Same here. > > > +#define IMX8M_RESET_MIPI_PHY_MRST IMX7_RESET_MIPI_PHY_MRST > > +#define IMX8M_RESET_MIPI_PHY_SRST IMX7_RESET_MIPI_PHY_SRST > > These don't exist either. The MIPIPHY_RCR register instead has 5 > different _RESET_N bits. > OK, I'll incorporate all of the feedback above in v2 as well. Thanks, Andrey Smirnov
next prev parent reply other threads:[~2018-11-26 15:59 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-11-17 18:11 [PATCH 0/1] Reset controller support for i.MX8MQ Andrey Smirnov 2018-11-17 18:11 ` Andrey Smirnov 2018-11-17 18:11 ` [PATCH 1/1] reset: imx7: Add " Andrey Smirnov 2018-11-17 18:11 ` Andrey Smirnov 2018-11-19 14:32 ` Philipp Zabel 2018-11-19 14:32 ` Philipp Zabel 2018-11-26 15:59 ` Andrey Smirnov [this message] 2018-11-26 15:59 ` Andrey Smirnov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAHQ1cqFEKHR3VW9gfaZcp-BJeh5PTM85F1kPF0am0rmaW9HiRQ@mail.gmail.com \ --to=andrew.smirnov@gmail.com \ --cc=aisheng.dong@nxp.com \ --cc=cphealy@gmail.com \ --cc=fabio.estevam@nxp.com \ --cc=hongxing.zhu@nxp.com \ --cc=l.stach@pengutronix.de \ --cc=leonard.crestez@nxp.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=p.zabel@pengutronix.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.