All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] reset: imx7: Add support for i.MX8MM SoC
@ 2019-07-01  9:39 ` Anson.Huang
  0 siblings, 0 replies; 11+ messages in thread
From: Anson.Huang @ 2019-07-01  9:39 UTC (permalink / raw)
  To: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
	p.zabel, leonard.crestez, viresh.kumar, daniel.baluta, ping.bai,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

From: Anson Huang <Anson.Huang@nxp.com>

i.MX8MM SoC has a subset of i.MX8MQ IP block variant, it can reuse
the i.MX8MQ reset controller driver and just skip those non-existing
IP blocks, add support for i.MX8MM SoC reset control.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/reset/reset-imx7.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
index 3ecd770..941131f 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -207,6 +207,26 @@ static int imx8mq_reset_set(struct reset_controller_dev *rcdev,
 	const unsigned int bit = imx7src->signals[id].bit;
 	unsigned int value = assert ? bit : 0;
 
+	if (of_machine_is_compatible("fsl,imx8mm")) {
+		switch (id) {
+		case IMX8MQ_RESET_HDMI_PHY_APB_RESET:
+		case IMX8MQ_RESET_PCIEPHY2: /* fallthrough */
+		case IMX8MQ_RESET_PCIEPHY2_PERST: /* fallthrough */
+		case IMX8MQ_RESET_PCIE2_CTRL_APPS_EN: /* fallthrough */
+		case IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI1_CORE_RESET: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI1_ESC_RESET: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI2_CORE_RESET: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI2_ESC_RESET: /* fallthrough */
+		case IMX8MQ_RESET_DDRC2_PHY_RESET: /* fallthrough */
+		case IMX8MQ_RESET_DDRC2_CORE_RESET: /* fallthrough */
+		case IMX8MQ_RESET_DDRC2_PRST: /* fallthrough */
+			return 0;
+		}
+	}
+
 	switch (id) {
 	case IMX8MQ_RESET_PCIEPHY:
 	case IMX8MQ_RESET_PCIEPHY2: /* fallthrough */
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 1/2] reset: imx7: Add support for i.MX8MM SoC
@ 2019-07-01  9:39 ` Anson.Huang
  0 siblings, 0 replies; 11+ messages in thread
From: Anson.Huang @ 2019-07-01  9:39 UTC (permalink / raw)
  To: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
	p.zabel, leonard.crestez, viresh.kumar, daniel.baluta, ping.bai,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

From: Anson Huang <Anson.Huang@nxp.com>

i.MX8MM SoC has a subset of i.MX8MQ IP block variant, it can reuse
the i.MX8MQ reset controller driver and just skip those non-existing
IP blocks, add support for i.MX8MM SoC reset control.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/reset/reset-imx7.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
index 3ecd770..941131f 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -207,6 +207,26 @@ static int imx8mq_reset_set(struct reset_controller_dev *rcdev,
 	const unsigned int bit = imx7src->signals[id].bit;
 	unsigned int value = assert ? bit : 0;
 
+	if (of_machine_is_compatible("fsl,imx8mm")) {
+		switch (id) {
+		case IMX8MQ_RESET_HDMI_PHY_APB_RESET:
+		case IMX8MQ_RESET_PCIEPHY2: /* fallthrough */
+		case IMX8MQ_RESET_PCIEPHY2_PERST: /* fallthrough */
+		case IMX8MQ_RESET_PCIE2_CTRL_APPS_EN: /* fallthrough */
+		case IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI1_CORE_RESET: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI1_ESC_RESET: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI2_CORE_RESET: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET: /* fallthrough */
+		case IMX8MQ_RESET_MIPI_CSI2_ESC_RESET: /* fallthrough */
+		case IMX8MQ_RESET_DDRC2_PHY_RESET: /* fallthrough */
+		case IMX8MQ_RESET_DDRC2_CORE_RESET: /* fallthrough */
+		case IMX8MQ_RESET_DDRC2_PRST: /* fallthrough */
+			return 0;
+		}
+	}
+
 	switch (id) {
 	case IMX8MQ_RESET_PCIEPHY:
 	case IMX8MQ_RESET_PCIEPHY2: /* fallthrough */
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 2/2] arm64: dts: imx8mm: Add "fsl,imx8mq-src" as src's fallback compatible
  2019-07-01  9:39 ` Anson.Huang
@ 2019-07-01  9:39   ` Anson.Huang
  -1 siblings, 0 replies; 11+ messages in thread
From: Anson.Huang @ 2019-07-01  9:39 UTC (permalink / raw)
  To: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
	p.zabel, leonard.crestez, viresh.kumar, daniel.baluta, ping.bai,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

From: Anson Huang <Anson.Huang@nxp.com>

i.MX8MM can reuse i.MX8MQ's src driver, add "fsl,imx8mq-src" as
src's fallback compatible to enable it.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index f0ac027..ea15457 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -471,7 +471,7 @@
 			};
 
 			src: reset-controller@30390000 {
-				compatible = "fsl,imx8mm-src", "syscon";
+				compatible = "fsl,imx8mm-src", "fsl,imx8mq-src", "syscon";
 				reg = <0x30390000 0x10000>;
 				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
 				#reset-cells = <1>;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 2/2] arm64: dts: imx8mm: Add "fsl, imx8mq-src" as src's fallback compatible
@ 2019-07-01  9:39   ` Anson.Huang
  0 siblings, 0 replies; 11+ messages in thread
From: Anson.Huang @ 2019-07-01  9:39 UTC (permalink / raw)
  To: robh+dt, mark.rutland, shawnguo, s.hauer, kernel, festevam,
	p.zabel, leonard.crestez, viresh.kumar, daniel.baluta, ping.bai,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

From: Anson Huang <Anson.Huang@nxp.com>

i.MX8MM can reuse i.MX8MQ's src driver, add "fsl,imx8mq-src" as
src's fallback compatible to enable it.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8mm.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
index f0ac027..ea15457 100644
--- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
@@ -471,7 +471,7 @@
 			};
 
 			src: reset-controller@30390000 {
-				compatible = "fsl,imx8mm-src", "syscon";
+				compatible = "fsl,imx8mm-src", "fsl,imx8mq-src", "syscon";
 				reg = <0x30390000 0x10000>;
 				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
 				#reset-cells = <1>;
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] reset: imx7: Add support for i.MX8MM SoC
  2019-07-01  9:39 ` Anson.Huang
@ 2019-07-04  8:54   ` Philipp Zabel
  -1 siblings, 0 replies; 11+ messages in thread
From: Philipp Zabel @ 2019-07-04  8:54 UTC (permalink / raw)
  To: Anson.Huang, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
	festevam, leonard.crestez, viresh.kumar, daniel.baluta, ping.bai,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

Hi Anson,

On Mon, 2019-07-01 at 17:39 +0800, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
> 
> i.MX8MM SoC has a subset of i.MX8MQ IP block variant, it can reuse
> the i.MX8MQ reset controller driver and just skip those non-existing
> IP blocks, add support for i.MX8MM SoC reset control.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/reset/reset-imx7.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 3ecd770..941131f 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -207,6 +207,26 @@ static int imx8mq_reset_set(struct reset_controller_dev *rcdev,
>  	const unsigned int bit = imx7src->signals[id].bit;
>  	unsigned int value = assert ? bit : 0;
>  
> +	if (of_machine_is_compatible("fsl,imx8mm")) {

This should be checked once during probe, not in every reset_set, if
this check has to be made at all. On i.MX8MM these unused reset controls
are not going to be hooked up via phandle, so this function should never
be called with the values that are filtered out here anyway.
And in case somebody makes an error in the device tree, does writing 1
to the reserved bits on i.MX8MM have any negative side effects at all?
Or are these bits just not hooked up? If this is no problem, I assume
this patch is not needed at all.

The correct way to protect against DT writers hooking up the non-
existing reset lines would be to replace rcdev.of_xlate with a version
that returns -EINVAL for them on i.MX8MM. Also in that case I'd check
the reset-controller compatible, not the machine compatible.

> +		switch (id) {
> +		case IMX8MQ_RESET_HDMI_PHY_APB_RESET:
> +		case IMX8MQ_RESET_PCIEPHY2: /* fallthrough */
> +		case IMX8MQ_RESET_PCIEPHY2_PERST: /* fallthrough */
> +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_EN: /* fallthrough */
> +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI1_CORE_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI1_ESC_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI2_CORE_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI2_ESC_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_DDRC2_PHY_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_DDRC2_CORE_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_DDRC2_PRST: /* fallthrough */

I think it would make sense to add IMX8MM_RESET_* defines for all but
these, to avoid confusion when reading the imx8mm.dtsi

regards
Philipp

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] reset: imx7: Add support for i.MX8MM SoC
@ 2019-07-04  8:54   ` Philipp Zabel
  0 siblings, 0 replies; 11+ messages in thread
From: Philipp Zabel @ 2019-07-04  8:54 UTC (permalink / raw)
  To: Anson.Huang, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
	festevam, leonard.crestez, viresh.kumar, daniel.baluta, ping.bai,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

Hi Anson,

On Mon, 2019-07-01 at 17:39 +0800, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
> 
> i.MX8MM SoC has a subset of i.MX8MQ IP block variant, it can reuse
> the i.MX8MQ reset controller driver and just skip those non-existing
> IP blocks, add support for i.MX8MM SoC reset control.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/reset/reset-imx7.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> index 3ecd770..941131f 100644
> --- a/drivers/reset/reset-imx7.c
> +++ b/drivers/reset/reset-imx7.c
> @@ -207,6 +207,26 @@ static int imx8mq_reset_set(struct reset_controller_dev *rcdev,
>  	const unsigned int bit = imx7src->signals[id].bit;
>  	unsigned int value = assert ? bit : 0;
>  
> +	if (of_machine_is_compatible("fsl,imx8mm")) {

This should be checked once during probe, not in every reset_set, if
this check has to be made at all. On i.MX8MM these unused reset controls
are not going to be hooked up via phandle, so this function should never
be called with the values that are filtered out here anyway.
And in case somebody makes an error in the device tree, does writing 1
to the reserved bits on i.MX8MM have any negative side effects at all?
Or are these bits just not hooked up? If this is no problem, I assume
this patch is not needed at all.

The correct way to protect against DT writers hooking up the non-
existing reset lines would be to replace rcdev.of_xlate with a version
that returns -EINVAL for them on i.MX8MM. Also in that case I'd check
the reset-controller compatible, not the machine compatible.

> +		switch (id) {
> +		case IMX8MQ_RESET_HDMI_PHY_APB_RESET:
> +		case IMX8MQ_RESET_PCIEPHY2: /* fallthrough */
> +		case IMX8MQ_RESET_PCIEPHY2_PERST: /* fallthrough */
> +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_EN: /* fallthrough */
> +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI1_CORE_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI1_ESC_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI2_CORE_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_MIPI_CSI2_ESC_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_DDRC2_PHY_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_DDRC2_CORE_RESET: /* fallthrough */
> +		case IMX8MQ_RESET_DDRC2_PRST: /* fallthrough */

I think it would make sense to add IMX8MM_RESET_* defines for all but
these, to avoid confusion when reading the imx8mm.dtsi

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] reset: imx7: Add support for i.MX8MM SoC
  2019-07-04  8:54   ` Philipp Zabel
  (?)
@ 2019-07-04  9:09     ` Anson Huang
  -1 siblings, 0 replies; 11+ messages in thread
From: Anson Huang @ 2019-07-04  9:09 UTC (permalink / raw)
  To: Philipp Zabel, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
	festevam, Leonard Crestez, viresh.kumar, Daniel Baluta,
	Jacky Bai, devicetree, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

Hi, Philipp

> On Mon, 2019-07-01 at 17:39 +0800, Anson.Huang@nxp.com wrote:
> > From: Anson Huang <Anson.Huang@nxp.com>
> >
> > i.MX8MM SoC has a subset of i.MX8MQ IP block variant, it can reuse the
> > i.MX8MQ reset controller driver and just skip those non-existing IP
> > blocks, add support for i.MX8MM SoC reset control.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/reset/reset-imx7.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> > index 3ecd770..941131f 100644
> > --- a/drivers/reset/reset-imx7.c
> > +++ b/drivers/reset/reset-imx7.c
> > @@ -207,6 +207,26 @@ static int imx8mq_reset_set(struct
> reset_controller_dev *rcdev,
> >  	const unsigned int bit = imx7src->signals[id].bit;
> >  	unsigned int value = assert ? bit : 0;
> >
> > +	if (of_machine_is_compatible("fsl,imx8mm")) {
> 
> This should be checked once during probe, not in every reset_set, if this
> check has to be made at all. On i.MX8MM these unused reset controls are
> not going to be hooked up via phandle, so this function should never be
> called with the values that are filtered out here anyway.
> And in case somebody makes an error in the device tree, does writing 1 to
> the reserved bits on i.MX8MM have any negative side effects at all?
> Or are these bits just not hooked up? If this is no problem, I assume this
> patch is not needed at all.

I just tried it on i.MX8MM, read/write to the reserved registers looks like OK,
so this patch can be ignored, although accessing reserved registers is NOT good,
but the user should have correct settings in DT, there will be no access for these
reserved registers.

So, let's just ignore this patch, adding the fallback compatible should be good for
i.MX8MM to reuse this driver.

Thanks,
Anson 

> 
> The correct way to protect against DT writers hooking up the non- existing
> reset lines would be to replace rcdev.of_xlate with a version that returns -
> EINVAL for them on i.MX8MM. Also in that case I'd check the reset-controller
> compatible, not the machine compatible.
> 
> > +		switch (id) {
> > +		case IMX8MQ_RESET_HDMI_PHY_APB_RESET:
> > +		case IMX8MQ_RESET_PCIEPHY2: /* fallthrough */
> > +		case IMX8MQ_RESET_PCIEPHY2_PERST: /* fallthrough */
> > +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_EN: /* fallthrough
> */
> > +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF: /*
> fallthrough */
> > +		case IMX8MQ_RESET_MIPI_CSI1_CORE_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET: /*
> fallthrough */
> > +		case IMX8MQ_RESET_MIPI_CSI1_ESC_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_MIPI_CSI2_CORE_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET: /*
> fallthrough */
> > +		case IMX8MQ_RESET_MIPI_CSI2_ESC_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_DDRC2_PHY_RESET: /* fallthrough */
> > +		case IMX8MQ_RESET_DDRC2_CORE_RESET: /* fallthrough */
> > +		case IMX8MQ_RESET_DDRC2_PRST: /* fallthrough */
> 
> I think it would make sense to add IMX8MM_RESET_* defines for all but
> these, to avoid confusion when reading the imx8mm.dtsi
> 
> regards
> Philipp

^ permalink raw reply	[flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] reset: imx7: Add support for i.MX8MM SoC
@ 2019-07-04  9:09     ` Anson Huang
  0 siblings, 0 replies; 11+ messages in thread
From: Anson Huang @ 2019-07-04  9:09 UTC (permalink / raw)
  To: Philipp Zabel, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
	festevam, Leonard Crestez, viresh.kumar, Daniel Baluta,
	Jacky Bai, devicetree, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

Hi, Philipp

> On Mon, 2019-07-01 at 17:39 +0800, Anson.Huang@nxp.com wrote:
> > From: Anson Huang <Anson.Huang@nxp.com>
> >
> > i.MX8MM SoC has a subset of i.MX8MQ IP block variant, it can reuse the
> > i.MX8MQ reset controller driver and just skip those non-existing IP
> > blocks, add support for i.MX8MM SoC reset control.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/reset/reset-imx7.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> > index 3ecd770..941131f 100644
> > --- a/drivers/reset/reset-imx7.c
> > +++ b/drivers/reset/reset-imx7.c
> > @@ -207,6 +207,26 @@ static int imx8mq_reset_set(struct
> reset_controller_dev *rcdev,
> >  	const unsigned int bit = imx7src->signals[id].bit;
> >  	unsigned int value = assert ? bit : 0;
> >
> > +	if (of_machine_is_compatible("fsl,imx8mm")) {
> 
> This should be checked once during probe, not in every reset_set, if this
> check has to be made at all. On i.MX8MM these unused reset controls are
> not going to be hooked up via phandle, so this function should never be
> called with the values that are filtered out here anyway.
> And in case somebody makes an error in the device tree, does writing 1 to
> the reserved bits on i.MX8MM have any negative side effects at all?
> Or are these bits just not hooked up? If this is no problem, I assume this
> patch is not needed at all.

I just tried it on i.MX8MM, read/write to the reserved registers looks like OK,
so this patch can be ignored, although accessing reserved registers is NOT good,
but the user should have correct settings in DT, there will be no access for these
reserved registers.

So, let's just ignore this patch, adding the fallback compatible should be good for
i.MX8MM to reuse this driver.

Thanks,
Anson 

> 
> The correct way to protect against DT writers hooking up the non- existing
> reset lines would be to replace rcdev.of_xlate with a version that returns -
> EINVAL for them on i.MX8MM. Also in that case I'd check the reset-controller
> compatible, not the machine compatible.
> 
> > +		switch (id) {
> > +		case IMX8MQ_RESET_HDMI_PHY_APB_RESET:
> > +		case IMX8MQ_RESET_PCIEPHY2: /* fallthrough */
> > +		case IMX8MQ_RESET_PCIEPHY2_PERST: /* fallthrough */
> > +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_EN: /* fallthrough
> */
> > +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF: /*
> fallthrough */
> > +		case IMX8MQ_RESET_MIPI_CSI1_CORE_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET: /*
> fallthrough */
> > +		case IMX8MQ_RESET_MIPI_CSI1_ESC_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_MIPI_CSI2_CORE_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET: /*
> fallthrough */
> > +		case IMX8MQ_RESET_MIPI_CSI2_ESC_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_DDRC2_PHY_RESET: /* fallthrough */
> > +		case IMX8MQ_RESET_DDRC2_CORE_RESET: /* fallthrough */
> > +		case IMX8MQ_RESET_DDRC2_PRST: /* fallthrough */
> 
> I think it would make sense to add IMX8MM_RESET_* defines for all but
> these, to avoid confusion when reading the imx8mm.dtsi
> 
> regards
> Philipp

^ permalink raw reply	[flat|nested] 11+ messages in thread
* RE: [PATCH 1/2] reset: imx7: Add support for i.MX8MM SoC
@ 2019-07-04  9:09     ` Anson Huang
  0 siblings, 0 replies; 11+ messages in thread
From: Anson Huang @ 2019-07-04  9:09 UTC (permalink / raw)
  To: Philipp Zabel, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
	festevam, Leonard Crestez, viresh.kumar, Daniel Baluta,
	Jacky Bai, devicetree, linux-arm-kernel, linux-kernel
  Cc: dl-linux-imx

Hi, Philipp

> On Mon, 2019-07-01 at 17:39 +0800, Anson.Huang@nxp.com wrote:
> > From: Anson Huang <Anson.Huang@nxp.com>
> >
> > i.MX8MM SoC has a subset of i.MX8MQ IP block variant, it can reuse the
> > i.MX8MQ reset controller driver and just skip those non-existing IP
> > blocks, add support for i.MX8MM SoC reset control.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/reset/reset-imx7.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/reset/reset-imx7.c b/drivers/reset/reset-imx7.c
> > index 3ecd770..941131f 100644
> > --- a/drivers/reset/reset-imx7.c
> > +++ b/drivers/reset/reset-imx7.c
> > @@ -207,6 +207,26 @@ static int imx8mq_reset_set(struct
> reset_controller_dev *rcdev,
> >  	const unsigned int bit = imx7src->signals[id].bit;
> >  	unsigned int value = assert ? bit : 0;
> >
> > +	if (of_machine_is_compatible("fsl,imx8mm")) {
> 
> This should be checked once during probe, not in every reset_set, if this
> check has to be made at all. On i.MX8MM these unused reset controls are
> not going to be hooked up via phandle, so this function should never be
> called with the values that are filtered out here anyway.
> And in case somebody makes an error in the device tree, does writing 1 to
> the reserved bits on i.MX8MM have any negative side effects at all?
> Or are these bits just not hooked up? If this is no problem, I assume this
> patch is not needed at all.

I just tried it on i.MX8MM, read/write to the reserved registers looks like OK,
so this patch can be ignored, although accessing reserved registers is NOT good,
but the user should have correct settings in DT, there will be no access for these
reserved registers.

So, let's just ignore this patch, adding the fallback compatible should be good for
i.MX8MM to reuse this driver.

Thanks,
Anson 

> 
> The correct way to protect against DT writers hooking up the non- existing
> reset lines would be to replace rcdev.of_xlate with a version that returns -
> EINVAL for them on i.MX8MM. Also in that case I'd check the reset-controller
> compatible, not the machine compatible.
> 
> > +		switch (id) {
> > +		case IMX8MQ_RESET_HDMI_PHY_APB_RESET:
> > +		case IMX8MQ_RESET_PCIEPHY2: /* fallthrough */
> > +		case IMX8MQ_RESET_PCIEPHY2_PERST: /* fallthrough */
> > +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_EN: /* fallthrough
> */
> > +		case IMX8MQ_RESET_PCIE2_CTRL_APPS_TURNOFF: /*
> fallthrough */
> > +		case IMX8MQ_RESET_MIPI_CSI1_CORE_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_MIPI_CSI1_PHY_REF_RESET: /*
> fallthrough */
> > +		case IMX8MQ_RESET_MIPI_CSI1_ESC_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_MIPI_CSI2_CORE_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_MIPI_CSI2_PHY_REF_RESET: /*
> fallthrough */
> > +		case IMX8MQ_RESET_MIPI_CSI2_ESC_RESET: /* fallthrough
> */
> > +		case IMX8MQ_RESET_DDRC2_PHY_RESET: /* fallthrough */
> > +		case IMX8MQ_RESET_DDRC2_CORE_RESET: /* fallthrough */
> > +		case IMX8MQ_RESET_DDRC2_PRST: /* fallthrough */
> 
> I think it would make sense to add IMX8MM_RESET_* defines for all but
> these, to avoid confusion when reading the imx8mm.dtsi
> 
> regards
> Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] arm64: dts: imx8mm: Add "fsl,imx8mq-src" as src's fallback compatible
  2019-07-01  9:39   ` [PATCH 2/2] arm64: dts: imx8mm: Add "fsl, imx8mq-src" " Anson.Huang
@ 2019-07-04  9:15     ` Philipp Zabel
  -1 siblings, 0 replies; 11+ messages in thread
From: Philipp Zabel @ 2019-07-04  9:15 UTC (permalink / raw)
  To: Anson.Huang, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
	festevam, leonard.crestez, viresh.kumar, daniel.baluta, ping.bai,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

Hi Anson,

On Mon, 2019-07-01 at 17:39 +0800, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
> 
> i.MX8MM can reuse i.MX8MQ's src driver, add "fsl,imx8mq-src" as
> src's fallback compatible to enable it.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index f0ac027..ea15457 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -471,7 +471,7 @@
>  			};
>  
>  			src: reset-controller@30390000 {
> -				compatible = "fsl,imx8mm-src", "syscon";
> +				compatible = "fsl,imx8mm-src", "fsl,imx8mq-src", "syscon";
>  				reg = <0x30390000 0x10000>;
>  				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
>  				#reset-cells = <1>;

Please also update the compatible property documentation in
Documentation/devicetree/bindings/reset/fsl,imx7-src.txt.
With that,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] arm64: dts: imx8mm: Add "fsl,imx8mq-src" as src's fallback compatible
@ 2019-07-04  9:15     ` Philipp Zabel
  0 siblings, 0 replies; 11+ messages in thread
From: Philipp Zabel @ 2019-07-04  9:15 UTC (permalink / raw)
  To: Anson.Huang, robh+dt, mark.rutland, shawnguo, s.hauer, kernel,
	festevam, leonard.crestez, viresh.kumar, daniel.baluta, ping.bai,
	devicetree, linux-arm-kernel, linux-kernel
  Cc: Linux-imx

Hi Anson,

On Mon, 2019-07-01 at 17:39 +0800, Anson.Huang@nxp.com wrote:
> From: Anson Huang <Anson.Huang@nxp.com>
> 
> i.MX8MM can reuse i.MX8MQ's src driver, add "fsl,imx8mq-src" as
> src's fallback compatible to enable it.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mm.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm.dtsi b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> index f0ac027..ea15457 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mm.dtsi
> @@ -471,7 +471,7 @@
>  			};
>  
>  			src: reset-controller@30390000 {
> -				compatible = "fsl,imx8mm-src", "syscon";
> +				compatible = "fsl,imx8mm-src", "fsl,imx8mq-src", "syscon";
>  				reg = <0x30390000 0x10000>;
>  				interrupts = <GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>;
>  				#reset-cells = <1>;

Please also update the compatible property documentation in
Documentation/devicetree/bindings/reset/fsl,imx7-src.txt.
With that,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-07-04  9:15 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-01  9:39 [PATCH 1/2] reset: imx7: Add support for i.MX8MM SoC Anson.Huang
2019-07-01  9:39 ` Anson.Huang
2019-07-01  9:39 ` [PATCH 2/2] arm64: dts: imx8mm: Add "fsl,imx8mq-src" as src's fallback compatible Anson.Huang
2019-07-01  9:39   ` [PATCH 2/2] arm64: dts: imx8mm: Add "fsl, imx8mq-src" " Anson.Huang
2019-07-04  9:15   ` [PATCH 2/2] arm64: dts: imx8mm: Add "fsl,imx8mq-src" " Philipp Zabel
2019-07-04  9:15     ` Philipp Zabel
2019-07-04  8:54 ` [PATCH 1/2] reset: imx7: Add support for i.MX8MM SoC Philipp Zabel
2019-07-04  8:54   ` Philipp Zabel
2019-07-04  9:09   ` Anson Huang
2019-07-04  9:09     ` Anson Huang
2019-07-04  9:09     ` Anson Huang

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.