Re: [PATCH v4 2/3] dt-bindings: reset: imx7: Document usage on i.MX8MQ SoCs

[Andrey Smirnov]

On Thu, Jan 17, 2019 at 8:45 AM Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx
> > Cc: Fabio Estevam <fabio.estevam@xxxxxxx>
> > Cc: cphealy@xxxxxxxxx
> > Cc: l.stach@xxxxxxxxxxxxxx
> > Cc: Leonard Crestez <leonard.crestez@xxxxxxx>
> > Cc: "A.s. Dong" <aisheng.dong@xxxxxxx>
> > Cc: Richard Zhu <hongxing.zhu@xxxxxxx>
> > Cc: linux-imx@xxxxxxx
> > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Reviewed-by: Rob Herring <robh@xxxxxxxxxx>
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxx>
> > + */
> > +
> > +#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