| Message ID | 1593425623-31810-2-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] reset: imx7: Support module build | expand |
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
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.
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
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
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
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
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
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
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(+)