[PATCH 1/3] reset: imx7: Support module build

[PATCH 1/3] reset: imx7: Support module build

[Anson Huang]

Add module device table, module license to support module build.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/reset/Kconfig      | 4 ++--
 drivers/reset/reset-imx7.c | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index d9efbfd..033ab60 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -65,9 +65,9 @@ config RESET_HSDK
 	  This enables the reset controller driver for HSDK board.
 
 config RESET_IMX7
-	bool "i.MX7/8 Reset Driver" if COMPILE_TEST
+	tristate "i.MX7/8 Reset Driver"
 	depends on HAS_IOMEM
-	default SOC_IMX7D || (ARM64 && ARCH_MXC)
+	depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || COMPILE_TEST
 	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 d170fe6..4660c93 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -8,7 +8,7 @@
  */
 
 #include <linux/mfd/syscon.h>
-#include <linux/mod_devicetable.h>
+#include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/reset-controller.h>
@@ -386,6 +386,7 @@ static const struct of_device_id imx7_reset_dt_ids[] = {
 	{ .compatible = "fsl,imx8mp-src", .data = &variant_imx8mp },
 	{ /* sentinel */ },
 };
+MODULE_DEVICE_TABLE(of, imx7_reset_dt_ids);
 
 static struct platform_driver imx7_reset_driver = {
 	.probe	= imx7_reset_probe,
@@ -395,3 +396,4 @@ static struct platform_driver imx7_reset_driver = {
 	},
 };
 builtin_platform_driver(imx7_reset_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH 1/3] reset: imx7: Support module build

[Anson Huang]

Add module device table, module license to support module build.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/reset/Kconfig      | 4 ++--
 drivers/reset/reset-imx7.c | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index d9efbfd..033ab60 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -65,9 +65,9 @@ config RESET_HSDK
 	  This enables the reset controller driver for HSDK board.
 
 config RESET_IMX7
-	bool "i.MX7/8 Reset Driver" if COMPILE_TEST
+	tristate "i.MX7/8 Reset Driver"
 	depends on HAS_IOMEM
-	default SOC_IMX7D || (ARM64 && ARCH_MXC)
+	depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || COMPILE_TEST
 	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 d170fe6..4660c93 100644
--- a/drivers/reset/reset-imx7.c
+++ b/drivers/reset/reset-imx7.c
@@ -8,7 +8,7 @@
  */
 
 #include <linux/mfd/syscon.h>
-#include <linux/mod_devicetable.h>
+#include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/reset-controller.h>
@@ -386,6 +386,7 @@ static const struct of_device_id imx7_reset_dt_ids[] = {
 	{ .compatible = "fsl,imx8mp-src", .data = &variant_imx8mp },
 	{ /* sentinel */ },
 };
+MODULE_DEVICE_TABLE(of, imx7_reset_dt_ids);
 
 static struct platform_driver imx7_reset_driver = {
 	.probe	= imx7_reset_probe,
@@ -395,3 +396,4 @@ static struct platform_driver imx7_reset_driver = {
 	},
 };
 builtin_platform_driver(imx7_reset_driver);
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


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

[PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Anson Huang]

i.MX7 reset driver now supports module build, it is no longer
built in by default, need to select it explicitly.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 35f037f..84a29ec 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -909,6 +909,7 @@ CONFIG_PWM_SAMSUNG=y
 CONFIG_PWM_SUN4I=m
 CONFIG_PWM_TEGRA=m
 CONFIG_QCOM_PDC=y
+CONFIG_RESET_IMX7=y
 CONFIG_RESET_QCOM_AOSS=y
 CONFIG_RESET_QCOM_PDC=m
 CONFIG_RESET_TI_SCI=y
-- 
2.7.4

[PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Anson Huang]

i.MX7 reset driver now supports module build, it is no longer
built in by default, need to select it explicitly.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 35f037f..84a29ec 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -909,6 +909,7 @@ CONFIG_PWM_SAMSUNG=y
 CONFIG_PWM_SUN4I=m
 CONFIG_PWM_TEGRA=m
 CONFIG_QCOM_PDC=y
+CONFIG_RESET_IMX7=y
 CONFIG_RESET_QCOM_AOSS=y
 CONFIG_RESET_QCOM_PDC=m
 CONFIG_RESET_TI_SCI=y
-- 
2.7.4


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

[PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Anson Huang]

i.MX7 reset driver now supports module build, it is no longer
built in by default, need to select it explicitly for i.MX7D.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm/mach-imx/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index e7d7b90..3540483 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -561,6 +561,7 @@ config SOC_IMX7D_CM4
 config SOC_IMX7D
 	bool "i.MX7 Dual support"
 	select PINCTRL_IMX7D
+	select RESET_IMX7
 	select SOC_IMX7D_CA7 if ARCH_MULTI_V7
 	select SOC_IMX7D_CM4 if ARM_SINGLE_ARMV7M
 	select ARM_ERRATA_814220 if ARCH_MULTI_V7
-- 
2.7.4

[PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Anson Huang]

i.MX7 reset driver now supports module build, it is no longer
built in by default, need to select it explicitly for i.MX7D.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm/mach-imx/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index e7d7b90..3540483 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -561,6 +561,7 @@ config SOC_IMX7D_CM4
 config SOC_IMX7D
 	bool "i.MX7 Dual support"
 	select PINCTRL_IMX7D
+	select RESET_IMX7
 	select SOC_IMX7D_CA7 if ARCH_MULTI_V7
 	select SOC_IMX7D_CM4 if ARM_SINGLE_ARMV7M
 	select ARM_ERRATA_814220 if ARCH_MULTI_V7
-- 
2.7.4


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

Re: [PATCH 1/3] reset: imx7: Support module build

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> Add module device table, module license to support module build.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/reset/Kconfig      | 4 ++--
>  drivers/reset/reset-imx7.c | 4 +++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index d9efbfd..033ab60 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -65,9 +65,9 @@ config RESET_HSDK
>           This enables the reset controller driver for HSDK board.
>
>  config RESET_IMX7
> -       bool "i.MX7/8 Reset Driver" if COMPILE_TEST
> +       tristate "i.MX7/8 Reset Driver"
>         depends on HAS_IOMEM
> -       default SOC_IMX7D || (ARM64 && ARCH_MXC)
> +       depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || COMPILE_TEST
>         select MFD_SYSCON

You are dropping the 'default' line, so the driver is now disabled
in a defconfig build, which is not mentioned in the patch description.

Maybe make it 'default m'?

config RESET_IMX7
       tristate "i.MX7/8 Reset Driver"
       depends on HAS_IOMEM
       depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || COMPILE_TEST
       default m if (SOC_IMX7D || (ARM64 && ARCH_MXC))
       select MFD_SYSCON

> @@ -395,3 +396,4 @@ static struct platform_driver imx7_reset_driver = {
>         },
>  };
>  builtin_platform_driver(imx7_reset_driver);
> +MODULE_LICENSE("GPL v2");

Generally speaking: when you add a MODULE_LICENSE tag, please also
add MODULE_AUTHOR and MODULE_DESCRIPTION.

The 'builtin_platform_driver()' should work correctly but prevent unloading the
module. Ideally please changed to 'module_platform_driver()' and add a
.remove function for the platform_driver.


       Arnd

Re: [PATCH 1/3] reset: imx7: Support module build

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> Add module device table, module license to support module build.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  drivers/reset/Kconfig      | 4 ++--
>  drivers/reset/reset-imx7.c | 4 +++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index d9efbfd..033ab60 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -65,9 +65,9 @@ config RESET_HSDK
>           This enables the reset controller driver for HSDK board.
>
>  config RESET_IMX7
> -       bool "i.MX7/8 Reset Driver" if COMPILE_TEST
> +       tristate "i.MX7/8 Reset Driver"
>         depends on HAS_IOMEM
> -       default SOC_IMX7D || (ARM64 && ARCH_MXC)
> +       depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || COMPILE_TEST
>         select MFD_SYSCON

You are dropping the 'default' line, so the driver is now disabled
in a defconfig build, which is not mentioned in the patch description.

Maybe make it 'default m'?

config RESET_IMX7
       tristate "i.MX7/8 Reset Driver"
       depends on HAS_IOMEM
       depends on SOC_IMX7D || (ARM64 && ARCH_MXC) || COMPILE_TEST
       default m if (SOC_IMX7D || (ARM64 && ARCH_MXC))
       select MFD_SYSCON

> @@ -395,3 +396,4 @@ static struct platform_driver imx7_reset_driver = {
>         },
>  };
>  builtin_platform_driver(imx7_reset_driver);
> +MODULE_LICENSE("GPL v2");

Generally speaking: when you add a MODULE_LICENSE tag, please also
add MODULE_AUTHOR and MODULE_DESCRIPTION.

The 'builtin_platform_driver()' should work correctly but prevent unloading the
module. Ideally please changed to 'module_platform_driver()' and add a
.remove function for the platform_driver.


       Arnd

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

RE: [PATCH 1/3] reset: imx7: Support module build

[Anson Huang]

> Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> 
> On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> >
> > Add module device table, module license to support module build.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/reset/Kconfig      | 4 ++--
> >  drivers/reset/reset-imx7.c | 4 +++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index
> > d9efbfd..033ab60 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -65,9 +65,9 @@ config RESET_HSDK
> >           This enables the reset controller driver for HSDK board.
> >
> >  config RESET_IMX7
> > -       bool "i.MX7/8 Reset Driver" if COMPILE_TEST
> > +       tristate "i.MX7/8 Reset Driver"
> >         depends on HAS_IOMEM
> > -       default SOC_IMX7D || (ARM64 && ARCH_MXC)
> > +       depends on SOC_IMX7D || (ARM64 && ARCH_MXC) ||
> COMPILE_TEST
> >         select MFD_SYSCON
> 
> You are dropping the 'default' line, so the driver is now disabled in a defconfig
> build, which is not mentioned in the patch description.
> 
> Maybe make it 'default m'?
> 
> config RESET_IMX7
>        tristate "i.MX7/8 Reset Driver"
>        depends on HAS_IOMEM
>        depends on SOC_IMX7D || (ARM64 && ARCH_MXC) ||
> COMPILE_TEST
>        default m if (SOC_IMX7D || (ARM64 && ARCH_MXC))
>        select MFD_SYSCON
> 
> > @@ -395,3 +396,4 @@ static struct platform_driver imx7_reset_driver = {
> >         },
> >  };
> >  builtin_platform_driver(imx7_reset_driver);
> > +MODULE_LICENSE("GPL v2");
> 
> Generally speaking: when you add a MODULE_LICENSE tag, please also add
> MODULE_AUTHOR and MODULE_DESCRIPTION.
> 

OK, will add them in V2.

> The 'builtin_platform_driver()' should work correctly but prevent unloading
> the module. Ideally please changed to 'module_platform_driver()' and add
> a .remove function for the platform_driver.
> 

The reset driver normally won't be removed since it is necessary for drivers which
need it, it is just for Android GKI support, in this case, do we need to change it to
module_platform_driver()? 

Thanks,
Anson

RE: [PATCH 1/3] reset: imx7: Support module build

[Anson Huang]

> Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> 
> On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> >
> > Add module device table, module license to support module build.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  drivers/reset/Kconfig      | 4 ++--
> >  drivers/reset/reset-imx7.c | 4 +++-
> >  2 files changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig index
> > d9efbfd..033ab60 100644
> > --- a/drivers/reset/Kconfig
> > +++ b/drivers/reset/Kconfig
> > @@ -65,9 +65,9 @@ config RESET_HSDK
> >           This enables the reset controller driver for HSDK board.
> >
> >  config RESET_IMX7
> > -       bool "i.MX7/8 Reset Driver" if COMPILE_TEST
> > +       tristate "i.MX7/8 Reset Driver"
> >         depends on HAS_IOMEM
> > -       default SOC_IMX7D || (ARM64 && ARCH_MXC)
> > +       depends on SOC_IMX7D || (ARM64 && ARCH_MXC) ||
> COMPILE_TEST
> >         select MFD_SYSCON
> 
> You are dropping the 'default' line, so the driver is now disabled in a defconfig
> build, which is not mentioned in the patch description.
> 
> Maybe make it 'default m'?
> 
> config RESET_IMX7
>        tristate "i.MX7/8 Reset Driver"
>        depends on HAS_IOMEM
>        depends on SOC_IMX7D || (ARM64 && ARCH_MXC) ||
> COMPILE_TEST
>        default m if (SOC_IMX7D || (ARM64 && ARCH_MXC))
>        select MFD_SYSCON
> 
> > @@ -395,3 +396,4 @@ static struct platform_driver imx7_reset_driver = {
> >         },
> >  };
> >  builtin_platform_driver(imx7_reset_driver);
> > +MODULE_LICENSE("GPL v2");
> 
> Generally speaking: when you add a MODULE_LICENSE tag, please also add
> MODULE_AUTHOR and MODULE_DESCRIPTION.
> 

OK, will add them in V2.

> The 'builtin_platform_driver()' should work correctly but prevent unloading
> the module. Ideally please changed to 'module_platform_driver()' and add
> a .remove function for the platform_driver.
> 

The reset driver normally won't be removed since it is necessary for drivers which
need it, it is just for Android GKI support, in this case, do we need to change it to
module_platform_driver()? 

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

Re: [PATCH 1/3] reset: imx7: Support module build

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 12:45 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> The reset driver normally won't be removed since it is necessary for drivers which
> need it, it is just for Android GKI support, in this case, do we need to change it to
> module_platform_driver()?

Please at least try to do it, or explain in the changelog what went wrong
if it doesn't work.

I don't think "because Android GKI" should ever be the sole justification for
a patch. Making drivers loadable module is a good idea regardless of GKI,
and if you do that, then make it behave like any other loadable module.

        Arnd

Re: [PATCH 1/3] reset: imx7: Support module build

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 12:45 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> The reset driver normally won't be removed since it is necessary for drivers which
> need it, it is just for Android GKI support, in this case, do we need to change it to
> module_platform_driver()?

Please at least try to do it, or explain in the changelog what went wrong
if it doesn't work.

I don't think "because Android GKI" should ever be the sole justification for
a patch. Making drivers loadable module is a good idea regardless of GKI,
and if you do that, then make it behave like any other loadable module.

        Arnd

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

Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> i.MX7 reset driver now supports module build, it is no longer
> built in by default, need to select it explicitly.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Why not make it =m now that this is possible?

      Arnd

Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> i.MX7 reset driver now supports module build, it is no longer
> built in by default, need to select it explicitly.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>

Why not make it =m now that this is possible?

      Arnd

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

Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> i.MX7 reset driver now supports module build, it is no longer
> built in by default, need to select it explicitly for i.MX7D.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  arch/arm/mach-imx/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index e7d7b90..3540483 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -561,6 +561,7 @@ config SOC_IMX7D_CM4
>  config SOC_IMX7D
>         bool "i.MX7 Dual support"
>         select PINCTRL_IMX7D
> +       select RESET_IMX7

Please drop this patch, it breaks the intent of the first one by forcing
the driver to be built-in again rather than a loadable module.

        Arnd

Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
>
> i.MX7 reset driver now supports module build, it is no longer
> built in by default, need to select it explicitly for i.MX7D.
>
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  arch/arm/mach-imx/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> index e7d7b90..3540483 100644
> --- a/arch/arm/mach-imx/Kconfig
> +++ b/arch/arm/mach-imx/Kconfig
> @@ -561,6 +561,7 @@ config SOC_IMX7D_CM4
>  config SOC_IMX7D
>         bool "i.MX7 Dual support"
>         select PINCTRL_IMX7D
> +       select RESET_IMX7

Please drop this patch, it breaks the intent of the first one by forcing
the driver to be built-in again rather than a loadable module.

        Arnd

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

RE: [PATCH 1/3] reset: imx7: Support module build

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> 
> On Mon, Jun 29, 2020 at 12:45 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build On Mon,
> > > Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
> >
> > The reset driver normally won't be removed since it is necessary for
> > drivers which need it, it is just for Android GKI support, in this
> > case, do we need to change it to module_platform_driver()?
> 
> Please at least try to do it, or explain in the changelog what went wrong if it
> doesn't work.
> 
> I don't think "because Android GKI" should ever be the sole justification for a
> patch. Making drivers loadable module is a good idea regardless of GKI, and if
> you do that, then make it behave like any other loadable module.
> 

OK, will do it in V2, BTW, I think there is nothing need to be done for .remove() callback,
can I just skip it or need to add a blank callback anyway?

Thanks,
Anson

RE: [PATCH 1/3] reset: imx7: Support module build

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> 
> On Mon, Jun 29, 2020 at 12:45 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build On Mon,
> > > Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
> >
> > The reset driver normally won't be removed since it is necessary for
> > drivers which need it, it is just for Android GKI support, in this
> > case, do we need to change it to module_platform_driver()?
> 
> Please at least try to do it, or explain in the changelog what went wrong if it
> doesn't work.
> 
> I don't think "because Android GKI" should ever be the sole justification for a
> patch. Making drivers loadable module is a good idea regardless of GKI, and if
> you do that, then make it behave like any other loadable module.
> 

OK, will do it in V2, BTW, I think there is nothing need to be done for .remove() callback,
can I just skip it or need to add a blank callback anyway?

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

RE: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by
> default
> 
> On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> >
> > i.MX7 reset driver now supports module build, it is no longer built in
> > by default, need to select it explicitly.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> 
> Why not make it =m now that this is possible?
> 

It is because some drivers depends on this reset driver to work, such as PCIe,
If by default make it =m, it may impact PCIe's function, adding module support
at this point is try to provide function of loadable module for Android, but don't
want to impact any function which is working previously.

Thanks
Anson.

RE: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by
> default
> 
> On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> >
> > i.MX7 reset driver now supports module build, it is no longer built in
> > by default, need to select it explicitly.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> 
> Why not make it =m now that this is possible?
> 

It is because some drivers depends on this reset driver to work, such as PCIe,
If by default make it =m, it may impact PCIe's function, adding module support
at this point is try to provide function of loadable module for Android, but don't
want to impact any function which is working previously.

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

RE: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> 
> On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> >
> > i.MX7 reset driver now supports module build, it is no longer built in
> > by default, need to select it explicitly for i.MX7D.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  arch/arm/mach-imx/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > index e7d7b90..3540483 100644
> > --- a/arch/arm/mach-imx/Kconfig
> > +++ b/arch/arm/mach-imx/Kconfig
> > @@ -561,6 +561,7 @@ config SOC_IMX7D_CM4  config SOC_IMX7D
> >         bool "i.MX7 Dual support"
> >         select PINCTRL_IMX7D
> > +       select RESET_IMX7
> 
> Please drop this patch, it breaks the intent of the first one by forcing the driver
> to be built-in again rather than a loadable module.
> 

One thing to clarify is, the loadable module support is ONLY for i.MX ARMv8 SoCs,
ARMv7 SoCs is NOT in the plan, that is why I add this patch to make it same as previously
when this reset driver is =y by default. Without this patch, this reset driver will NOT be
enabled by default for i.MX ARMv7 SoCs, function will be broken.

Thanks,
Anson

RE: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> 
> On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> >
> > i.MX7 reset driver now supports module build, it is no longer built in
> > by default, need to select it explicitly for i.MX7D.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  arch/arm/mach-imx/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > index e7d7b90..3540483 100644
> > --- a/arch/arm/mach-imx/Kconfig
> > +++ b/arch/arm/mach-imx/Kconfig
> > @@ -561,6 +561,7 @@ config SOC_IMX7D_CM4  config SOC_IMX7D
> >         bool "i.MX7 Dual support"
> >         select PINCTRL_IMX7D
> > +       select RESET_IMX7
> 
> Please drop this patch, it breaks the intent of the first one by forcing the driver
> to be built-in again rather than a loadable module.
> 

One thing to clarify is, the loadable module support is ONLY for i.MX ARMv8 SoCs,
ARMv7 SoCs is NOT in the plan, that is why I add this patch to make it same as previously
when this reset driver is =y by default. Without this patch, this reset driver will NOT be
enabled by default for i.MX ARMv7 SoCs, function will be broken.

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

Re: [PATCH 1/3] reset: imx7: Support module build

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 1:32 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> >
> > On Mon, Jun 29, 2020 at 12:45 PM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build On Mon,
> > > > Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
> > >
> > > The reset driver normally won't be removed since it is necessary for
> > > drivers which need it, it is just for Android GKI support, in this
> > > case, do we need to change it to module_platform_driver()?
> >
> > Please at least try to do it, or explain in the changelog what went wrong if it
> > doesn't work.
> >
> > I don't think "because Android GKI" should ever be the sole justification for a
> > patch. Making drivers loadable module is a good idea regardless of GKI, and if
> > you do that, then make it behave like any other loadable module.
> >
>
> OK, will do it in V2, BTW, I think there is nothing need to be done for .remove() callback,
> can I just skip it or need to add a blank callback anyway?

I think if everything is done through devm_() calls you should be able to
have no .remove() callback at all, but I have not tried that.

     Arnd

Re: [PATCH 1/3] reset: imx7: Support module build

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 1:32 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> >
> > On Mon, Jun 29, 2020 at 12:45 PM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build On Mon,
> > > > Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com> wrote:
> > >
> > > The reset driver normally won't be removed since it is necessary for
> > > drivers which need it, it is just for Android GKI support, in this
> > > case, do we need to change it to module_platform_driver()?
> >
> > Please at least try to do it, or explain in the changelog what went wrong if it
> > doesn't work.
> >
> > I don't think "because Android GKI" should ever be the sole justification for a
> > patch. Making drivers loadable module is a good idea regardless of GKI, and if
> > you do that, then make it behave like any other loadable module.
> >
>
> OK, will do it in V2, BTW, I think there is nothing need to be done for .remove() callback,
> can I just skip it or need to add a blank callback anyway?

I think if everything is done through devm_() calls you should be able to
have no .remove() callback at all, but I have not tried that.

     Arnd

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

RE: [PATCH 1/3] reset: imx7: Support module build

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> 
> On Mon, Jun 29, 2020 at 1:32 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> > >
> > > On Mon, Jun 29, 2020 at 12:45 PM Anson Huang
> <anson.huang@nxp.com>
> > > wrote:
> > > > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build On
> > > > > Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> > > >
> > > > The reset driver normally won't be removed since it is necessary
> > > > for drivers which need it, it is just for Android GKI support, in
> > > > this case, do we need to change it to module_platform_driver()?
> > >
> > > Please at least try to do it, or explain in the changelog what went
> > > wrong if it doesn't work.
> > >
> > > I don't think "because Android GKI" should ever be the sole
> > > justification for a patch. Making drivers loadable module is a good
> > > idea regardless of GKI, and if you do that, then make it behave like any
> other loadable module.
> > >
> >
> > OK, will do it in V2, BTW, I think there is nothing need to be done
> > for .remove() callback, can I just skip it or need to add a blank callback
> anyway?
> 
> I think if everything is done through devm_() calls you should be able to have
> no .remove() callback at all, but I have not tried that.
> 

OK, I have confirmed that before in other drivers, so I will not have .remove callback()
for this driver.

Thanks,
Anson

RE: [PATCH 1/3] reset: imx7: Support module build

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> 
> On Mon, Jun 29, 2020 at 1:32 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build
> > >
> > > On Mon, Jun 29, 2020 at 12:45 PM Anson Huang
> <anson.huang@nxp.com>
> > > wrote:
> > > > > Subject: Re: [PATCH 1/3] reset: imx7: Support module build On
> > > > > Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> > > >
> > > > The reset driver normally won't be removed since it is necessary
> > > > for drivers which need it, it is just for Android GKI support, in
> > > > this case, do we need to change it to module_platform_driver()?
> > >
> > > Please at least try to do it, or explain in the changelog what went
> > > wrong if it doesn't work.
> > >
> > > I don't think "because Android GKI" should ever be the sole
> > > justification for a patch. Making drivers loadable module is a good
> > > idea regardless of GKI, and if you do that, then make it behave like any
> other loadable module.
> > >
> >
> > OK, will do it in V2, BTW, I think there is nothing need to be done
> > for .remove() callback, can I just skip it or need to add a blank callback
> anyway?
> 
> I think if everything is done through devm_() calls you should be able to have
> no .remove() callback at all, but I have not tried that.
> 

OK, I have confirmed that before in other drivers, so I will not have .remove callback()
for this driver.

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

Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 1:36 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> >
> > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> > wrote:
> > >
> > > i.MX7 reset driver now supports module build, it is no longer built in
> > > by default, need to select it explicitly for i.MX7D.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > ---
> > >  arch/arm/mach-imx/Kconfig | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > > index e7d7b90..3540483 100644
> > > --- a/arch/arm/mach-imx/Kconfig
> > > +++ b/arch/arm/mach-imx/Kconfig
> > > @@ -561,6 +561,7 @@ config SOC_IMX7D_CM4  config SOC_IMX7D
> > >         bool "i.MX7 Dual support"
> > >         select PINCTRL_IMX7D
> > > +       select RESET_IMX7
> >
> > Please drop this patch, it breaks the intent of the first one by forcing the driver
> > to be built-in again rather than a loadable module.
> >
>
> One thing to clarify is, the loadable module support is ONLY for i.MX ARMv8 SoCs,

I don't understand. Why can't this driver be a loadable module on i.MX7?
Please clarify in the changelog what the bug is on i.MX7.

> ARMv7 SoCs is NOT in the plan, that is why I add this patch to make it same as previously
> when this reset driver is =y by default. Without this patch, this reset driver will NOT be
> enabled by default for i.MX ARMv7 SoCs, function will be broken.

But it's the same driver, make it work the same way on all chips!

I think artificially limiting a new driver feature to one chip just
because nobody
asked for it on another chip is not helpful.

      Arnd

Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 1:36 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> >
> > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> > wrote:
> > >
> > > i.MX7 reset driver now supports module build, it is no longer built in
> > > by default, need to select it explicitly for i.MX7D.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > ---
> > >  arch/arm/mach-imx/Kconfig | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > > index e7d7b90..3540483 100644
> > > --- a/arch/arm/mach-imx/Kconfig
> > > +++ b/arch/arm/mach-imx/Kconfig
> > > @@ -561,6 +561,7 @@ config SOC_IMX7D_CM4  config SOC_IMX7D
> > >         bool "i.MX7 Dual support"
> > >         select PINCTRL_IMX7D
> > > +       select RESET_IMX7
> >
> > Please drop this patch, it breaks the intent of the first one by forcing the driver
> > to be built-in again rather than a loadable module.
> >
>
> One thing to clarify is, the loadable module support is ONLY for i.MX ARMv8 SoCs,

I don't understand. Why can't this driver be a loadable module on i.MX7?
Please clarify in the changelog what the bug is on i.MX7.

> ARMv7 SoCs is NOT in the plan, that is why I add this patch to make it same as previously
> when this reset driver is =y by default. Without this patch, this reset driver will NOT be
> enabled by default for i.MX ARMv7 SoCs, function will be broken.

But it's the same driver, make it work the same way on all chips!

I think artificially limiting a new driver feature to one chip just
because nobody
asked for it on another chip is not helpful.

      Arnd

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

Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 1:34 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by
> > default
> >
> > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> > wrote:
> > >
> > > i.MX7 reset driver now supports module build, it is no longer built in
> > > by default, need to select it explicitly.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> >
> > Why not make it =m now that this is possible?
>
> It is because some drivers depends on this reset driver to work, such as PCIe,
> If by default make it =m, it may impact PCIe's function, adding module support
> at this point is try to provide function of loadable module for Android, but don't
> want to impact any function which is working previously.

It sounds like your patch 1/3 is not ready to be merged then.

Please make sure that loading it later does not break other drivers
that depend on it. Other drivers don't have to be able to deal with
missing dependencies if this one is never loaded or disabled at
compile-time. However before you make it possible to turn this into
a loadable module, anything that depends on it must be able to
deal with the modules getting loaded in a random order first.

       Arnd

Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 1:34 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by
> > default
> >
> > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang <Anson.Huang@nxp.com>
> > wrote:
> > >
> > > i.MX7 reset driver now supports module build, it is no longer built in
> > > by default, need to select it explicitly.
> > >
> > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> >
> > Why not make it =m now that this is possible?
>
> It is because some drivers depends on this reset driver to work, such as PCIe,
> If by default make it =m, it may impact PCIe's function, adding module support
> at this point is try to provide function of loadable module for Android, but don't
> want to impact any function which is working previously.

It sounds like your patch 1/3 is not ready to be merged then.

Please make sure that loading it later does not break other drivers
that depend on it. Other drivers don't have to be able to deal with
missing dependencies if this one is never loaded or disabled at
compile-time. However before you make it possible to turn this into
a loadable module, anything that depends on it must be able to
deal with the modules getting loaded in a random order first.

       Arnd

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

RE: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> 
> On Mon, Jun 29, 2020 at 1:36 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> > >
> > > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang
> <Anson.Huang@nxp.com>
> > > wrote:
> > > >
> > > > i.MX7 reset driver now supports module build, it is no longer
> > > > built in by default, need to select it explicitly for i.MX7D.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > > ---
> > > >  arch/arm/mach-imx/Kconfig | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > > > index e7d7b90..3540483 100644
> > > > --- a/arch/arm/mach-imx/Kconfig
> > > > +++ b/arch/arm/mach-imx/Kconfig
> > > > @@ -561,6 +561,7 @@ config SOC_IMX7D_CM4  config SOC_IMX7D
> > > >         bool "i.MX7 Dual support"
> > > >         select PINCTRL_IMX7D
> > > > +       select RESET_IMX7
> > >
> > > Please drop this patch, it breaks the intent of the first one by
> > > forcing the driver to be built-in again rather than a loadable module.
> > >
> >
> > One thing to clarify is, the loadable module support is ONLY for i.MX
> > ARMv8 SoCs,
> 
> I don't understand. Why can't this driver be a loadable module on i.MX7?
> Please clarify in the changelog what the bug is on i.MX7.
> 
> > ARMv7 SoCs is NOT in the plan, that is why I add this patch to make it
> > same as previously when this reset driver is =y by default. Without
> > this patch, this reset driver will NOT be enabled by default for i.MX ARMv7
> SoCs, function will be broken.
> 
> But it's the same driver, make it work the same way on all chips!
> 
> I think artificially limiting a new driver feature to one chip just because nobody
> asked for it on another chip is not helpful.
> 

Understood, what if I don't select it in SOC config which is NOT flexible, but put it in
imx_v6_v7_defconfig, then user can select either =y or =m according their requirement?
And in imx_v6_v7_defconfig make it =y by default.

Thanks,
Anson

RE: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> 
> On Mon, Jun 29, 2020 at 1:36 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> > >
> > > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang
> <Anson.Huang@nxp.com>
> > > wrote:
> > > >
> > > > i.MX7 reset driver now supports module build, it is no longer
> > > > built in by default, need to select it explicitly for i.MX7D.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > > > ---
> > > >  arch/arm/mach-imx/Kconfig | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
> > > > index e7d7b90..3540483 100644
> > > > --- a/arch/arm/mach-imx/Kconfig
> > > > +++ b/arch/arm/mach-imx/Kconfig
> > > > @@ -561,6 +561,7 @@ config SOC_IMX7D_CM4  config SOC_IMX7D
> > > >         bool "i.MX7 Dual support"
> > > >         select PINCTRL_IMX7D
> > > > +       select RESET_IMX7
> > >
> > > Please drop this patch, it breaks the intent of the first one by
> > > forcing the driver to be built-in again rather than a loadable module.
> > >
> >
> > One thing to clarify is, the loadable module support is ONLY for i.MX
> > ARMv8 SoCs,
> 
> I don't understand. Why can't this driver be a loadable module on i.MX7?
> Please clarify in the changelog what the bug is on i.MX7.
> 
> > ARMv7 SoCs is NOT in the plan, that is why I add this patch to make it
> > same as previously when this reset driver is =y by default. Without
> > this patch, this reset driver will NOT be enabled by default for i.MX ARMv7
> SoCs, function will be broken.
> 
> But it's the same driver, make it work the same way on all chips!
> 
> I think artificially limiting a new driver feature to one chip just because nobody
> asked for it on another chip is not helpful.
> 

Understood, what if I don't select it in SOC config which is NOT flexible, but put it in
imx_v6_v7_defconfig, then user can select either =y or =m according their requirement?
And in imx_v6_v7_defconfig make it =y by default.

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

Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 1:54 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> >
> > On Mon, Jun 29, 2020 at 1:36 PM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > > > Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> > > > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang
> > <Anson.Huang@nxp.com> wrote:

> > I don't understand. Why can't this driver be a loadable module on i.MX7?
> > Please clarify in the changelog what the bug is on i.MX7.
> >
> > > ARMv7 SoCs is NOT in the plan, that is why I add this patch to make it
> > > same as previously when this reset driver is =y by default. Without
> > > this patch, this reset driver will NOT be enabled by default for i.MX ARMv7
> > SoCs, function will be broken.
> >
> > But it's the same driver, make it work the same way on all chips!
> >
> > I think artificially limiting a new driver feature to one chip just because nobody
> > asked for it on another chip is not helpful.
> >
>
> Understood, what if I don't select it in SOC config which is NOT flexible, but put it in
> imx_v6_v7_defconfig, then user can select either =y or =m according their requirement?

I would recommend using a 'default m if SOC_IMX7' or 'default y if SOC_IMX7', so
any distros and users that have their own defconfigs still get a reasonable
default, but can choose between =m and =y based on their needs.

> And in imx_v6_v7_defconfig make it =y by default.

Having =y in imx_v6_v7_defconfig sounds reasonable if you go with 'default m'.

     Arnd

Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 1:54 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> >
> > On Mon, Jun 29, 2020 at 1:36 PM Anson Huang <anson.huang@nxp.com>
> > wrote:
> > > > Subject: Re: [PATCH 3/3] ARM: imx: Select RESET_IMX7 for i.MX7D
> > > > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang
> > <Anson.Huang@nxp.com> wrote:

> > I don't understand. Why can't this driver be a loadable module on i.MX7?
> > Please clarify in the changelog what the bug is on i.MX7.
> >
> > > ARMv7 SoCs is NOT in the plan, that is why I add this patch to make it
> > > same as previously when this reset driver is =y by default. Without
> > > this patch, this reset driver will NOT be enabled by default for i.MX ARMv7
> > SoCs, function will be broken.
> >
> > But it's the same driver, make it work the same way on all chips!
> >
> > I think artificially limiting a new driver feature to one chip just because nobody
> > asked for it on another chip is not helpful.
> >
>
> Understood, what if I don't select it in SOC config which is NOT flexible, but put it in
> imx_v6_v7_defconfig, then user can select either =y or =m according their requirement?

I would recommend using a 'default m if SOC_IMX7' or 'default y if SOC_IMX7', so
any distros and users that have their own defconfigs still get a reasonable
default, but can choose between =m and =y based on their needs.

> And in imx_v6_v7_defconfig make it =y by default.

Having =y in imx_v6_v7_defconfig sounds reasonable if you go with 'default m'.

     Arnd

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

RE: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by
> default
> 
> On Mon, Jun 29, 2020 at 1:34 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7
> > > by default
> > >
> > > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang
> <Anson.Huang@nxp.com>
> > > wrote:
> > > >
> > > > i.MX7 reset driver now supports module build, it is no longer
> > > > built in by default, need to select it explicitly.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > >
> > > Why not make it =m now that this is possible?
> >
> > It is because some drivers depends on this reset driver to work, such
> > as PCIe, If by default make it =m, it may impact PCIe's function,
> > adding module support at this point is try to provide function of
> > loadable module for Android, but don't want to impact any function which is
> working previously.
> 
> It sounds like your patch 1/3 is not ready to be merged then.
> 
> Please make sure that loading it later does not break other drivers that depend
> on it. Other drivers don't have to be able to deal with missing dependencies if
> this one is never loaded or disabled at compile-time. However before you
> make it possible to turn this into a loadable module, anything that depends on
> it must be able to deal with the modules getting loaded in a random order
> first.
> 
I searched all driver which uses this reset driver, looks like ONLY i.MX6 PCIe is using it and
it ONLY supports built-in, and inside this driver, it does NOT support defer probe etc.,
since I am NOT sure when this PCIe driver will add module support, so do you think if I
can make PCI_IMX6 select RESET_IMX7, then it won't break the PCIe function even
RESET_IMX7 is set to =m in defconfig, as when PCI_IMX6 is enabled as =y, RESET_IMX7 will
be also =y.

config PCI_IMX6
        bool "Freescale i.MX6/7/8 PCIe controller"
        depends on ARCH_MXC || COMPILE_TEST
        depends on PCI_MSI_IRQ_DOMAIN
        select PCIE_DW_HOST

Thanks,
Anson

RE: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Anson Huang]

Hi, Arnd


> Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by
> default
> 
> On Mon, Jun 29, 2020 at 1:34 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > > Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7
> > > by default
> > >
> > > On Mon, Jun 29, 2020 at 12:25 PM Anson Huang
> <Anson.Huang@nxp.com>
> > > wrote:
> > > >
> > > > i.MX7 reset driver now supports module build, it is no longer
> > > > built in by default, need to select it explicitly.
> > > >
> > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > >
> > > Why not make it =m now that this is possible?
> >
> > It is because some drivers depends on this reset driver to work, such
> > as PCIe, If by default make it =m, it may impact PCIe's function,
> > adding module support at this point is try to provide function of
> > loadable module for Android, but don't want to impact any function which is
> working previously.
> 
> It sounds like your patch 1/3 is not ready to be merged then.
> 
> Please make sure that loading it later does not break other drivers that depend
> on it. Other drivers don't have to be able to deal with missing dependencies if
> this one is never loaded or disabled at compile-time. However before you
> make it possible to turn this into a loadable module, anything that depends on
> it must be able to deal with the modules getting loaded in a random order
> first.
> 
I searched all driver which uses this reset driver, looks like ONLY i.MX6 PCIe is using it and
it ONLY supports built-in, and inside this driver, it does NOT support defer probe etc.,
since I am NOT sure when this PCIe driver will add module support, so do you think if I
can make PCI_IMX6 select RESET_IMX7, then it won't break the PCIe function even
RESET_IMX7 is set to =m in defconfig, as when PCI_IMX6 is enabled as =y, RESET_IMX7 will
be also =y.

config PCI_IMX6
        bool "Freescale i.MX6/7/8 PCIe controller"
        depends on ARCH_MXC || COMPILE_TEST
        depends on PCI_MSI_IRQ_DOMAIN
        select PCIE_DW_HOST

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

Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 2:09 PM Anson Huang <anson.huang@nxp.com> wrote:
> >
> > It sounds like your patch 1/3 is not ready to be merged then.
> >
> > Please make sure that loading it later does not break other drivers that depend
> > on it. Other drivers don't have to be able to deal with missing dependencies if
> > this one is never loaded or disabled at compile-time. However before you
> > make it possible to turn this into a loadable module, anything that depends on
> > it must be able to deal with the modules getting loaded in a random order
> > first.
> >
> I searched all driver which uses this reset driver, looks like ONLY i.MX6 PCIe is using it and
> it ONLY supports built-in.

Ok, thanks for researching this.

> and inside this driver, it does NOT support defer probe etc.,
> since I am NOT sure when this PCIe driver will add module support, so do you think if I
> can make PCI_IMX6 select RESET_IMX7, then it won't break the PCIe function even
> RESET_IMX7 is set to =m in defconfig, as when PCI_IMX6 is enabled as =y, RESET_IMX7 will
> be also =y.

Yes, I think this can work as a short-term workaround, though ideally the
PCIe driver would also become a loadable module and also support deferred
probing.

Having loadable PCIe drivers has traditionally been problematic in Linux,
but Rob Herring has recently improved this in the series containing patch
0c59c06a7c90 ("PCI: host-generic: Support building as modules"), which
was also intended to help with Android GKI.

As i.MX uses the designware PCI core support, this may require some
more changes in PCIE_DW before the i.MX specific part can be
a loadable module, but it should no longer require changes to the
PCI core code. Turning the driver into a loadable module is probably
not even that hard, but making it possible to unload definitely requires
adding a proper .remove callback to properly unregister the PCIe
host bridge.

I also see a DECLARE_PCI_FIXUP() and a fault handler hook in
the pci-imx6.c driver, which probably need to get moved into
a separate built-in file with a few changes.

And then I noticed a bug in the driver: it hooks the abort handler
from an initcall whenever the driver is built into the kernel, regardless
of which machine it is actually running on!

Regarding deferrer probing, the PCIe host currently relies on
the clk controller,  the regulator and (on imx7d) the reset driver
to be probed first. I think making it support deferred probing for
all three should be straightforward, most importantly this means
not printing an error and returning -EPROBE_DEFER when not
all dependencies are there yet.

       Arnd

Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 2:09 PM Anson Huang <anson.huang@nxp.com> wrote:
> >
> > It sounds like your patch 1/3 is not ready to be merged then.
> >
> > Please make sure that loading it later does not break other drivers that depend
> > on it. Other drivers don't have to be able to deal with missing dependencies if
> > this one is never loaded or disabled at compile-time. However before you
> > make it possible to turn this into a loadable module, anything that depends on
> > it must be able to deal with the modules getting loaded in a random order
> > first.
> >
> I searched all driver which uses this reset driver, looks like ONLY i.MX6 PCIe is using it and
> it ONLY supports built-in.

Ok, thanks for researching this.

> and inside this driver, it does NOT support defer probe etc.,
> since I am NOT sure when this PCIe driver will add module support, so do you think if I
> can make PCI_IMX6 select RESET_IMX7, then it won't break the PCIe function even
> RESET_IMX7 is set to =m in defconfig, as when PCI_IMX6 is enabled as =y, RESET_IMX7 will
> be also =y.

Yes, I think this can work as a short-term workaround, though ideally the
PCIe driver would also become a loadable module and also support deferred
probing.

Having loadable PCIe drivers has traditionally been problematic in Linux,
but Rob Herring has recently improved this in the series containing patch
0c59c06a7c90 ("PCI: host-generic: Support building as modules"), which
was also intended to help with Android GKI.

As i.MX uses the designware PCI core support, this may require some
more changes in PCIE_DW before the i.MX specific part can be
a loadable module, but it should no longer require changes to the
PCI core code. Turning the driver into a loadable module is probably
not even that hard, but making it possible to unload definitely requires
adding a proper .remove callback to properly unregister the PCIe
host bridge.

I also see a DECLARE_PCI_FIXUP() and a fault handler hook in
the pci-imx6.c driver, which probably need to get moved into
a separate built-in file with a few changes.

And then I noticed a bug in the driver: it hooks the abort handler
from an initcall whenever the driver is built into the kernel, regardless
of which machine it is actually running on!

Regarding deferrer probing, the PCIe host currently relies on
the clk controller,  the regulator and (on imx7d) the reset driver
to be probed first. I think making it support deferred probing for
all three should be straightforward, most importantly this means
not printing an error and returning -EPROBE_DEFER when not
all dependencies are there yet.

       Arnd

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

RE: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Anson Huang]

Hi, Arnd

> Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by
> default
> 
> On Mon, Jun 29, 2020 at 2:09 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > >
> > > It sounds like your patch 1/3 is not ready to be merged then.
> > >
> > > Please make sure that loading it later does not break other drivers
> > > that depend on it. Other drivers don't have to be able to deal with
> > > missing dependencies if this one is never loaded or disabled at
> > > compile-time. However before you make it possible to turn this into
> > > a loadable module, anything that depends on it must be able to deal
> > > with the modules getting loaded in a random order first.
> > >
> > I searched all driver which uses this reset driver, looks like ONLY
> > i.MX6 PCIe is using it and it ONLY supports built-in.
> 
> Ok, thanks for researching this.
> 
> > and inside this driver, it does NOT support defer probe etc., since I
> > am NOT sure when this PCIe driver will add module support, so do you
> > think if I can make PCI_IMX6 select RESET_IMX7, then it won't break
> > the PCIe function even
> > RESET_IMX7 is set to =m in defconfig, as when PCI_IMX6 is enabled as
> > =y, RESET_IMX7 will be also =y.
> 
> Yes, I think this can work as a short-term workaround, though ideally the PCIe
> driver would also become a loadable module and also support deferred
> probing.
> 
> Having loadable PCIe drivers has traditionally been problematic in Linux, but
> Rob Herring has recently improved this in the series containing patch
> 0c59c06a7c90 ("PCI: host-generic: Support building as modules"), which was
> also intended to help with Android GKI.
> 
> As i.MX uses the designware PCI core support, this may require some more
> changes in PCIE_DW before the i.MX specific part can be a loadable module,
> but it should no longer require changes to the PCI core code. Turning the driver
> into a loadable module is probably not even that hard, but making it possible
> to unload definitely requires adding a proper .remove callback to properly
> unregister the PCIe host bridge.
> 
> I also see a DECLARE_PCI_FIXUP() and a fault handler hook in the pci-imx6.c
> driver, which probably need to get moved into a separate built-in file with a
> few changes.
> 
> And then I noticed a bug in the driver: it hooks the abort handler from an
> initcall whenever the driver is built into the kernel, regardless of which
> machine it is actually running on!
> 
> Regarding deferrer probing, the PCIe host currently relies on the clk controller,
> the regulator and (on imx7d) the reset driver to be probed first. I think making
> it support deferred probing for all three should be straightforward, most
> importantly this means not printing an error and returning -EPROBE_DEFER
> when not all dependencies are there yet.
> 

Thanks for detailed info about PCI loadable module support, I copy our PCI owner
here to help on the support of i.MX6 PCIe driver to support loadable module. Meanwhile,
in this patch series, I will add the short-term workaround as I described upper, and then remove
this short-term workaround either in the i.MX6 PCI driver's loadable module support patch later
or adding a new patch if necessary.

Thanks,
Anson

RE: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Anson Huang]

Hi, Arnd

> Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by
> default
> 
> On Mon, Jun 29, 2020 at 2:09 PM Anson Huang <anson.huang@nxp.com>
> wrote:
> > >
> > > It sounds like your patch 1/3 is not ready to be merged then.
> > >
> > > Please make sure that loading it later does not break other drivers
> > > that depend on it. Other drivers don't have to be able to deal with
> > > missing dependencies if this one is never loaded or disabled at
> > > compile-time. However before you make it possible to turn this into
> > > a loadable module, anything that depends on it must be able to deal
> > > with the modules getting loaded in a random order first.
> > >
> > I searched all driver which uses this reset driver, looks like ONLY
> > i.MX6 PCIe is using it and it ONLY supports built-in.
> 
> Ok, thanks for researching this.
> 
> > and inside this driver, it does NOT support defer probe etc., since I
> > am NOT sure when this PCIe driver will add module support, so do you
> > think if I can make PCI_IMX6 select RESET_IMX7, then it won't break
> > the PCIe function even
> > RESET_IMX7 is set to =m in defconfig, as when PCI_IMX6 is enabled as
> > =y, RESET_IMX7 will be also =y.
> 
> Yes, I think this can work as a short-term workaround, though ideally the PCIe
> driver would also become a loadable module and also support deferred
> probing.
> 
> Having loadable PCIe drivers has traditionally been problematic in Linux, but
> Rob Herring has recently improved this in the series containing patch
> 0c59c06a7c90 ("PCI: host-generic: Support building as modules"), which was
> also intended to help with Android GKI.
> 
> As i.MX uses the designware PCI core support, this may require some more
> changes in PCIE_DW before the i.MX specific part can be a loadable module,
> but it should no longer require changes to the PCI core code. Turning the driver
> into a loadable module is probably not even that hard, but making it possible
> to unload definitely requires adding a proper .remove callback to properly
> unregister the PCIe host bridge.
> 
> I also see a DECLARE_PCI_FIXUP() and a fault handler hook in the pci-imx6.c
> driver, which probably need to get moved into a separate built-in file with a
> few changes.
> 
> And then I noticed a bug in the driver: it hooks the abort handler from an
> initcall whenever the driver is built into the kernel, regardless of which
> machine it is actually running on!
> 
> Regarding deferrer probing, the PCIe host currently relies on the clk controller,
> the regulator and (on imx7d) the reset driver to be probed first. I think making
> it support deferred probing for all three should be straightforward, most
> importantly this means not printing an error and returning -EPROBE_DEFER
> when not all dependencies are there yet.
> 

Thanks for detailed info about PCI loadable module support, I copy our PCI owner
here to help on the support of i.MX6 PCIe driver to support loadable module. Meanwhile,
in this patch series, I will add the short-term workaround as I described upper, and then remove
this short-term workaround either in the i.MX6 PCI driver's loadable module support patch later
or adding a new patch if necessary.

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

Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 2:46 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by
> > default

> Thanks for detailed info about PCI loadable module support, I copy our PCI owner
> here to help on the support of i.MX6 PCIe driver to support loadable module. Meanwhile,
> in this patch series, I will add the short-term workaround as I described upper, and then remove
> this short-term workaround either in the i.MX6 PCI driver's loadable module support patch later
> or adding a new patch if necessary.

Ok, sounds good. Thanks,

     Arnd

Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by default

[Arnd Bergmann]

On Mon, Jun 29, 2020 at 2:46 PM Anson Huang <anson.huang@nxp.com> wrote:
> > Subject: Re: [PATCH 2/3] arm64: defconfig: Select CONFIG_RESET_IMX7 by
> > default

> Thanks for detailed info about PCI loadable module support, I copy our PCI owner
> here to help on the support of i.MX6 PCIe driver to support loadable module. Meanwhile,
> in this patch series, I will add the short-term workaround as I described upper, and then remove
> this short-term workaround either in the i.MX6 PCI driver's loadable module support patch later
> or adding a new patch if necessary.

Ok, sounds good. Thanks,

     Arnd

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