| Message ID | 20181220010700.8598-3-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Reset controller support for i.MX8MQ | expand |
Hi Andrey, sorry for the delay. Thank you for the update, apart from the comments below, the list now looks to be complete. On Wed, 2018-12-19 at 17:06 -0800, Andrey Smirnov wrote: > The driver now supports i.MX8MQ, so update bindings accordingly. > > 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 > Reviewed-by: Rob Herring <robh@kernel.org> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > --- > .../bindings/reset/fsl,imx7-src.txt | 7 +- > include/dt-bindings/reset/imx8mq-reset.h | 64 +++++++++++++++++++ > 2 files changed, 69 insertions(+), 2 deletions(-) > create mode 100644 include/dt-bindings/reset/imx8mq-reset.h > > diff --git a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt > index 1ab1d109318e..2ecf33815d18 100644 > --- a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt > +++ b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt > @@ -5,7 +5,9 @@ Please also refer to reset.txt in this directory for common reset > controller binding usage. > > Required properties: > -- compatible: Should be "fsl,imx7d-src", "syscon" > +- compatible: > + - For i.MX7 SoCs should be "fsl,imx7d-src", "syscon" > + - For i.MX8MQ SoCs should be "fsl,imx8mq-src", "syscon" > - reg: should be register base and length as documented in the > datasheet > - interrupts: Should contain SRC interrupt > @@ -44,4 +46,5 @@ Example: > > > For list of all valid reset indicies see > -<dt-bindings/reset/imx7-reset.h> > +<dt-bindings/reset/imx7-reset.h> for i.MX7 and > +<dt-bindings/reset/imx8mq-reset.h> for i.MX8MQ > diff --git a/include/dt-bindings/reset/imx8mq-reset.h b/include/dt-bindings/reset/imx8mq-reset.h > new file mode 100644 > index 000000000000..57c592498aa0 > --- /dev/null > +++ b/include/dt-bindings/reset/imx8mq-reset.h > @@ -0,0 +1,64 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2018 Zodiac Inflight Innovations > + * > + * Author: Andrey Smirnov <andrew.smirnov@gmail.com> > + */ > + > +#ifndef DT_BINDING_RESET_IMX8MQ_H > +#define DT_BINDING_RESET_IMX8MQ_H > + > +#define IMX8MQ_RESET_A53_CORE_POR_RESET0 0 > +#define IMX8MQ_RESET_A53_CORE_POR_RESET1 1 > +#define IMX8MQ_RESET_A53_CORE_POR_RESET2 2 > +#define IMX8MQ_RESET_A53_CORE_POR_RESET3 3 > +#define IMX8MQ_RESET_A53_CORE_RESET0 4 > +#define IMX8MQ_RESET_A53_CORE_RESET1 5 > +#define IMX8MQ_RESET_A53_CORE_RESET2 6 > +#define IMX8MQ_RESET_A53_CORE_RESET3 7 > +#define IMX8MQ_RESET_A53_DBG_RESET0 8 > +#define IMX8MQ_RESET_A53_DBG_RESET1 9 > +#define IMX8MQ_RESET_A53_DBG_RESET2 10 > +#define IMX8MQ_RESET_A53_DBG_RESET3 11 > +#define IMX8MQ_RESET_A53_ETM_RESET0 12 > +#define IMX8MQ_RESET_A53_ETM_RESET1 13 > +#define IMX8MQ_RESET_A53_ETM_RESET2 14 > +#define IMX8MQ_RESET_A53_ETM_RESET3 15 > +#define IMX8MQ_RESET_A53_SOC_DBG_RESET 16 > +#define IMX8MQ_RESET_A53_L2RESET 17 > +#define IMX8MQ_RESET_SW_NON_SCLR_M4C_RST 18 ^^^^^^^^ This might be an implementation detail. The reset line is (SW_?)M4C_RST. I suppose the self-clearing SW_M4C_RST bit could be used to implement .reset() functionality for the same line if needed. What about the self-clearing SW_M4P_RST bit? Has that been left out on purpose? > +#define IMX8MQ_RESET_OTG1_PHY_RESET 19 > +#define IMX8MQ_RESET_OTG2_PHY_RESET 20 > +#define IMX8MQ_RESET_MIPI_DSI_RESET_BYTE_N 21 > +#define IMX8MQ_RESET_MIPI_DSI_RESET_N 22 > +#define IMX8MQ_RESET_MIPI_DIS_DPI_RESET_N 23 > +#define IMX8MQ_RESET_MIPI_DIS_ESC_RESET_N 24 > +#define IMX8MQ_RESET_MIPI_DIS_PCLK_RESET_N 25 > +#define IMX8MQ_RESET_PCIEPHY 26 > +#define IMX8MQ_RESET_PCIEPHY_PERST 27 > +#define IMX8MQ_RESET_PCIE_CTRL_APPS_EN 28 > +#define IMX8MQ_RESET_PCIE_CTRL_APPS_TURNOFF 29 To be honest, I don't like these two, I'm not convinced anymore that they actually qualify as reset signals. To me it looks like this is something that the PCIe glue code should handle via syscon like i.MX6. Leonard, Lucas, what do you think? > +#define IMX8MQ_RESET_HDMI_PHY_APB_RESET 30 > +#define IMX8MQ_RESET_DISP_RESET 31 > +#define IMX8MQ_RESET_GPU_RESET 32 > +#define IMX8MQ_RESET_VPU_RESET 33 > +#define IMX8MQ_RESET_PCIEPHY2 34 > +#define IMX8MQ_RESET_PCIEPHY2_PERST 35 > +#define IMX8MQ_RESET_PCIE2_CTRL_APPS_EN 36 > +#define IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF 37 Same issue as PCIe #1 > +#define IMX8MQ_RESET_MIPI_CSI1_CORE_RESET 38 > +#define IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET 39 > +#define IMX8MQ_RESET_MIPI_CSI1_ESC_RESET 40 > +#define IMX8MQ_RESET_MIPI_CSI2_CORE_RESET 41 > +#define IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET 42 > +#define IMX8MQ_RESET_MIPI_CSI2_ESC_RESET 43 > +#define IMX8MQ_RESET_DDRC1_PRST 44 > +#define IMX8MQ_RESET_DDRC1_CORE_RESET 45 > +#define IMX8MQ_RESET_DDRC1_PHY_RESET 46 Does anybody know what the DDRC1_PHY_PWROKIN bit is, right next to the PHY reset? > +#define IMX8MQ_RESET_DDRC2_PRST 47 > +#define IMX8MQ_RESET_DDRC2_CORE_RESET 48 > +#define IMX8MQ_RESET_DDRC2_PHY_RESET 49 > + > +#define IMX8MQ_RESET_NUM 50 > + > +#endif regards Philipp
On Thu, Jan 17, 2019 at 8:45 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > Hi Andrey, > > sorry for the delay. Thank you for the update, apart from the comments > below, the list now looks to be complete. > > On Wed, 2018-12-19 at 17:06 -0800, Andrey Smirnov wrote: > > The driver now supports i.MX8MQ, so update bindings accordingly. > > > > 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 > > Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > --- > > .../bindings/reset/fsl,imx7-src.txt | 7 +- > > include/dt-bindings/reset/imx8mq-reset.h | 64 +++++++++++++++++++ > > 2 files changed, 69 insertions(+), 2 deletions(-) > > create mode 100644 include/dt-bindings/reset/imx8mq-reset.h > > > > diff --git a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt > > index 1ab1d109318e..2ecf33815d18 100644 > > --- a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt > > +++ b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt > > @@ -5,7 +5,9 @@ Please also refer to reset.txt in this directory for common reset > > controller binding usage. > > > > Required properties: > > -- compatible: Should be "fsl,imx7d-src", "syscon" > > +- compatible: > > + - For i.MX7 SoCs should be "fsl,imx7d-src", "syscon" > > + - For i.MX8MQ SoCs should be "fsl,imx8mq-src", "syscon" > > - reg: should be register base and length as documented in the > > datasheet > > - interrupts: Should contain SRC interrupt > > @@ -44,4 +46,5 @@ Example: > > > > > > For list of all valid reset indicies see > > -<dt-bindings/reset/imx7-reset.h> > > +<dt-bindings/reset/imx7-reset.h> for i.MX7 and > > +<dt-bindings/reset/imx8mq-reset.h> for i.MX8MQ > > diff --git a/include/dt-bindings/reset/imx8mq-reset.h b/include/dt-bindings/reset/imx8mq-reset.h > > new file mode 100644 > > index 000000000000..57c592498aa0 > > --- /dev/null > > +++ b/include/dt-bindings/reset/imx8mq-reset.h > > @@ -0,0 +1,64 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2018 Zodiac Inflight Innovations > > + * > > + * Author: Andrey Smirnov <andrew.smirnov@gmail.com> > > + */ > > + > > +#ifndef DT_BINDING_RESET_IMX8MQ_H > > +#define DT_BINDING_RESET_IMX8MQ_H > > + > > +#define IMX8MQ_RESET_A53_CORE_POR_RESET0 0 > > +#define IMX8MQ_RESET_A53_CORE_POR_RESET1 1 > > +#define IMX8MQ_RESET_A53_CORE_POR_RESET2 2 > > +#define IMX8MQ_RESET_A53_CORE_POR_RESET3 3 > > +#define IMX8MQ_RESET_A53_CORE_RESET0 4 > > +#define IMX8MQ_RESET_A53_CORE_RESET1 5 > > +#define IMX8MQ_RESET_A53_CORE_RESET2 6 > > +#define IMX8MQ_RESET_A53_CORE_RESET3 7 > > +#define IMX8MQ_RESET_A53_DBG_RESET0 8 > > +#define IMX8MQ_RESET_A53_DBG_RESET1 9 > > +#define IMX8MQ_RESET_A53_DBG_RESET2 10 > > +#define IMX8MQ_RESET_A53_DBG_RESET3 11 > > +#define IMX8MQ_RESET_A53_ETM_RESET0 12 > > +#define IMX8MQ_RESET_A53_ETM_RESET1 13 > > +#define IMX8MQ_RESET_A53_ETM_RESET2 14 > > +#define IMX8MQ_RESET_A53_ETM_RESET3 15 > > +#define IMX8MQ_RESET_A53_SOC_DBG_RESET 16 > > +#define IMX8MQ_RESET_A53_L2RESET 17 > > +#define IMX8MQ_RESET_SW_NON_SCLR_M4C_RST 18 > ^^^^^^^^ > > This might be an implementation detail. The reset line is > (SW_?)M4C_RST. I suppose the self-clearing SW_M4C_RST bit could be used > to implement .reset() functionality for the same line if needed. > I was using literal bit names for reset lines. In this case this is a bit 0 of SRC_M4RCR. > What about the self-clearing SW_M4P_RST bit? Has that been left out on > purpose? Since they are self-clearing they'd require a separate .reset function. I have no use-case for them at this moment and no way to test it, so I left them out. > > > +#define IMX8MQ_RESET_OTG1_PHY_RESET 19 > > +#define IMX8MQ_RESET_OTG2_PHY_RESET 20 > > +#define IMX8MQ_RESET_MIPI_DSI_RESET_BYTE_N 21 > > +#define IMX8MQ_RESET_MIPI_DSI_RESET_N 22 > > +#define IMX8MQ_RESET_MIPI_DIS_DPI_RESET_N 23 > > +#define IMX8MQ_RESET_MIPI_DIS_ESC_RESET_N 24 > > +#define IMX8MQ_RESET_MIPI_DIS_PCLK_RESET_N 25 > > +#define IMX8MQ_RESET_PCIEPHY 26 > > +#define IMX8MQ_RESET_PCIEPHY_PERST 27 > > +#define IMX8MQ_RESET_PCIE_CTRL_APPS_EN 28 > > +#define IMX8MQ_RESET_PCIE_CTRL_APPS_TURNOFF 29 > > To be honest, I don't like these two, I'm not convinced anymore that > they actually qualify as reset signals. To me it looks like this is > something that the PCIe glue code should handle via syscon like i.MX6. > Leonard, Lucas, what do you think? OK, one thing to keep in mind about this is that those bits are already exposed for i.MX7D and I think (correct me if I am wrong) there's no going back there. PCIe driver already has the code to use those on i.MX7D and, due to high degree of similarity, i.MX8MQ actually re-uses the same codepath (at least for IMX8MQ_RESET_PCIE_CTRL_APPS_EN). Thanks, Andrey Smirnov
On Thu, 2019-01-17 at 14:38 -0800, Andrey Smirnov wrote: [...] > > To be honest, I don't like these two, I'm not convinced anymore that > > they actually qualify as reset signals. To me it looks like this is > > something that the PCIe glue code should handle via syscon like i.MX6. > > Leonard, Lucas, what do you think? > > OK, one thing to keep in mind about this is that those bits are > already exposed for i.MX7D and I think (correct me if I am wrong) > there's no going back there. That's not a reason to repeat the same mistake for i.MX8QM, but at the moment I'm still trying to figure out if it actually was a mistake. > PCIe driver already has the code to use > those on i.MX7D and, due to high degree of similarity, i.MX8MQ > actually re-uses the same codepath (at least for > IMX8MQ_RESET_PCIE_CTRL_APPS_EN). We can always switch to i.MX6-like direct syscon/GPR manipulation and just drop the resets from DT. Since if this is done, it should be done for i.MX7 as well, I see no reason for this issue to hold up your i.MX8M changes. regards Philipp
On Wed, Jan 23, 2019 at 2:52 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Thu, 2019-01-17 at 14:38 -0800, Andrey Smirnov wrote: > [...] > > > To be honest, I don't like these two, I'm not convinced anymore that > > > they actually qualify as reset signals. To me it looks like this is > > > something that the PCIe glue code should handle via syscon like i.MX6. > > > Leonard, Lucas, what do you think? > > > > OK, one thing to keep in mind about this is that those bits are > > already exposed for i.MX7D and I think (correct me if I am wrong) > > there's no going back there. > > That's not a reason to repeat the same mistake for i.MX8QM, but at the > moment I'm still trying to figure out if it actually was a mistake. > It absolutely is. Removing those resets will not meaningfully simplify maintenance burden for this driver (a one line change), but it will cause additional code churn on PCI side of things. You may not agree with me that it is a _good_ reason to not to remove those resets, but it is a reason nonetheless. > > PCIe driver already has the code to use > > those on i.MX7D and, due to high degree of similarity, i.MX8MQ > > actually re-uses the same codepath (at least for > > IMX8MQ_RESET_PCIE_CTRL_APPS_EN). > > We can always switch to i.MX6-like direct syscon/GPR manipulation and > just drop the resets from DT. > Since if this is done, it should be done for i.MX7 as well, I see no > reason for this issue to hold up your i.MX8M changes. > Cool, thanks! Thanks, Andrey Smirnov
Hi Andrey, On Wed, 2019-01-23 at 21:33 -0800, Andrey Smirnov wrote: > On Wed, Jan 23, 2019 at 2:52 AM Philipp Zabel <p.zabel@pengutronix.de> wrote: > > > > On Thu, 2019-01-17 at 14:38 -0800, Andrey Smirnov wrote: > > [...] > > > > To be honest, I don't like these two, I'm not convinced anymore that > > > > they actually qualify as reset signals. To me it looks like this is > > > > something that the PCIe glue code should handle via syscon like i.MX6. > > > > Leonard, Lucas, what do you think? > > > > > > OK, one thing to keep in mind about this is that those bits are > > > already exposed for i.MX7D and I think (correct me if I am wrong) > > > there's no going back there. > > > > That's not a reason to repeat the same mistake for i.MX8QM, but at the > > moment I'm still trying to figure out if it actually was a mistake. > > > > It absolutely is. Ok, that was a sloppy expression. I'm glad you got my meaning anyway. Of course it is a reason, but it's not a good one. > Removing those resets will not meaningfully simplify > maintenance burden for this driver (a one line change), I'm less worried about the maintenance burden on this reset driver and more about lying in the device tree description and possibly setting a bad precedent. > but it will cause additional code churn on PCI side of things. > You may not agree with me that it is a _good_ reason to not to remove > those resets, but it is a reason nonetheless. Exactly. Code churn in one driver implementation should not stop us from fixing device tree bindings (in a backwards compatible fashion). Mind you, this is all under my assumption that the bits in question do not control resets and should never have been described as resets in the device tree. regards Philipp
diff --git a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt index 1ab1d109318e..2ecf33815d18 100644 --- a/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt +++ b/Documentation/devicetree/bindings/reset/fsl,imx7-src.txt @@ -5,7 +5,9 @@ Please also refer to reset.txt in this directory for common reset controller binding usage. Required properties: -- compatible: Should be "fsl,imx7d-src", "syscon" +- compatible: + - For i.MX7 SoCs should be "fsl,imx7d-src", "syscon" + - For i.MX8MQ SoCs should be "fsl,imx8mq-src", "syscon" - reg: should be register base and length as documented in the datasheet - interrupts: Should contain SRC interrupt @@ -44,4 +46,5 @@ Example: For list of all valid reset indicies see -<dt-bindings/reset/imx7-reset.h> +<dt-bindings/reset/imx7-reset.h> for i.MX7 and +<dt-bindings/reset/imx8mq-reset.h> for i.MX8MQ diff --git a/include/dt-bindings/reset/imx8mq-reset.h b/include/dt-bindings/reset/imx8mq-reset.h new file mode 100644 index 000000000000..57c592498aa0 --- /dev/null +++ b/include/dt-bindings/reset/imx8mq-reset.h @@ -0,0 +1,64 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2018 Zodiac Inflight Innovations + * + * Author: Andrey Smirnov <andrew.smirnov@gmail.com> + */ + +#ifndef DT_BINDING_RESET_IMX8MQ_H +#define DT_BINDING_RESET_IMX8MQ_H + +#define IMX8MQ_RESET_A53_CORE_POR_RESET0 0 +#define IMX8MQ_RESET_A53_CORE_POR_RESET1 1 +#define IMX8MQ_RESET_A53_CORE_POR_RESET2 2 +#define IMX8MQ_RESET_A53_CORE_POR_RESET3 3 +#define IMX8MQ_RESET_A53_CORE_RESET0 4 +#define IMX8MQ_RESET_A53_CORE_RESET1 5 +#define IMX8MQ_RESET_A53_CORE_RESET2 6 +#define IMX8MQ_RESET_A53_CORE_RESET3 7 +#define IMX8MQ_RESET_A53_DBG_RESET0 8 +#define IMX8MQ_RESET_A53_DBG_RESET1 9 +#define IMX8MQ_RESET_A53_DBG_RESET2 10 +#define IMX8MQ_RESET_A53_DBG_RESET3 11 +#define IMX8MQ_RESET_A53_ETM_RESET0 12 +#define IMX8MQ_RESET_A53_ETM_RESET1 13 +#define IMX8MQ_RESET_A53_ETM_RESET2 14 +#define IMX8MQ_RESET_A53_ETM_RESET3 15 +#define IMX8MQ_RESET_A53_SOC_DBG_RESET 16 +#define IMX8MQ_RESET_A53_L2RESET 17 +#define IMX8MQ_RESET_SW_NON_SCLR_M4C_RST 18 +#define IMX8MQ_RESET_OTG1_PHY_RESET 19 +#define IMX8MQ_RESET_OTG2_PHY_RESET 20 +#define IMX8MQ_RESET_MIPI_DSI_RESET_BYTE_N 21 +#define IMX8MQ_RESET_MIPI_DSI_RESET_N 22 +#define IMX8MQ_RESET_MIPI_DIS_DPI_RESET_N 23 +#define IMX8MQ_RESET_MIPI_DIS_ESC_RESET_N 24 +#define IMX8MQ_RESET_MIPI_DIS_PCLK_RESET_N 25 +#define IMX8MQ_RESET_PCIEPHY 26 +#define IMX8MQ_RESET_PCIEPHY_PERST 27 +#define IMX8MQ_RESET_PCIE_CTRL_APPS_EN 28 +#define IMX8MQ_RESET_PCIE_CTRL_APPS_TURNOFF 29 +#define IMX8MQ_RESET_HDMI_PHY_APB_RESET 30 +#define IMX8MQ_RESET_DISP_RESET 31 +#define IMX8MQ_RESET_GPU_RESET 32 +#define IMX8MQ_RESET_VPU_RESET 33 +#define IMX8MQ_RESET_PCIEPHY2 34 +#define IMX8MQ_RESET_PCIEPHY2_PERST 35 +#define IMX8MQ_RESET_PCIE2_CTRL_APPS_EN 36 +#define IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF 37 +#define IMX8MQ_RESET_MIPI_CSI1_CORE_RESET 38 +#define IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET 39 +#define IMX8MQ_RESET_MIPI_CSI1_ESC_RESET 40 +#define IMX8MQ_RESET_MIPI_CSI2_CORE_RESET 41 +#define IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET 42 +#define IMX8MQ_RESET_MIPI_CSI2_ESC_RESET 43 +#define IMX8MQ_RESET_DDRC1_PRST 44 +#define IMX8MQ_RESET_DDRC1_CORE_RESET 45 +#define IMX8MQ_RESET_DDRC1_PHY_RESET 46 +#define IMX8MQ_RESET_DDRC2_PRST 47 +#define IMX8MQ_RESET_DDRC2_CORE_RESET 48 +#define IMX8MQ_RESET_DDRC2_PHY_RESET 49 + +#define IMX8MQ_RESET_NUM 50 + +#endif