* [PATCH] iommu: mtk: add common-clk dependency @ 2016-11-16 15:28 Arnd Bergmann 2016-11-16 19:38 ` Stephen Boyd 0 siblings, 1 reply; 5+ messages in thread From: Arnd Bergmann @ 2016-11-16 15:28 UTC (permalink / raw) To: Joerg Roedel, Matthias Brugger Cc: Shunli Wang, James Liao, Erin Lo, John Crispin, Stephen Boyd, Arnd Bergmann, iommu, linux-kernel, linux-arm-kernel, linux-mediatek After the MT2701 clock driver was added, we get a harmless warning for the iommu driver that selects it, when compile-testing without COMMON_CLK. warning: (MTK_IOMMU_V1) selects COMMON_CLK_MT2701_IMGSYS which has unmet direct dependencies (COMMON_CLK) Adding a dependency on COMMON_CLK avoids the warning. Fixes: e9862118272a ("clk: mediatek: Add MT2701 clock support") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/iommu/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index 8ee54d71c7eb..bb537d06d319 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -346,7 +346,7 @@ config MTK_IOMMU config MTK_IOMMU_V1 bool "MTK IOMMU Version 1 (M4U gen1) Support" - depends on ARM + depends on ARM && COMMON_CLK depends on ARCH_MEDIATEK || COMPILE_TEST select ARM_DMA_USE_IOMMU select IOMMU_API -- 2.9.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: mtk: add common-clk dependency 2016-11-16 15:28 [PATCH] iommu: mtk: add common-clk dependency Arnd Bergmann @ 2016-11-16 19:38 ` Stephen Boyd 2016-11-17 1:25 ` Honghui Zhang 0 siblings, 1 reply; 5+ messages in thread From: Stephen Boyd @ 2016-11-16 19:38 UTC (permalink / raw) To: Arnd Bergmann Cc: Joerg Roedel, Matthias Brugger, Shunli Wang, James Liao, Erin Lo, John Crispin, iommu, linux-kernel, linux-arm-kernel, linux-mediatek On 11/16, Arnd Bergmann wrote: > After the MT2701 clock driver was added, we get a harmless warning for > the iommu driver that selects it, when compile-testing without > COMMON_CLK. > > warning: (MTK_IOMMU_V1) selects COMMON_CLK_MT2701_IMGSYS which has unmet direct dependencies (COMMON_CLK) > > Adding a dependency on COMMON_CLK avoids the warning. > > Fixes: e9862118272a ("clk: mediatek: Add MT2701 clock support") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Hm.. why is an iommu driver selecting a clk driver? They should be using standard clk APIs so it's not like they need it for build time correctness. Shouldn't we drop the selects instead? Those look to have been introduced a few kernel versions ago, but they were selecting options that didn't exist until a few days ago when I merged the mediatek clk driver. The clk options are user-visible, so it should be possible to select them in the configuration phase. ----8<---- diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig index 8ee54d71c7eb..37e204f3d9be 100644 --- a/drivers/iommu/Kconfig +++ b/drivers/iommu/Kconfig @@ -352,9 +352,6 @@ config MTK_IOMMU_V1 select IOMMU_API select MEMORY select MTK_SMI - select COMMON_CLK_MT2701_MMSYS - select COMMON_CLK_MT2701_IMGSYS - select COMMON_CLK_MT2701_VDECSYS help Support for the M4U on certain Mediatek SoCs. M4U generation 1 HW is Multimedia Memory Managememt Unit. This option enables remapping of -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: mtk: add common-clk dependency 2016-11-16 19:38 ` Stephen Boyd @ 2016-11-17 1:25 ` Honghui Zhang 2016-11-17 23:35 ` Stephen Boyd 0 siblings, 1 reply; 5+ messages in thread From: Honghui Zhang @ 2016-11-17 1:25 UTC (permalink / raw) To: Stephen Boyd Cc: Arnd Bergmann, James Liao, linux-kernel, iommu, linux-mediatek, Shunli Wang, Matthias Brugger, Erin Lo, linux-arm-kernel, John Crispin On Wed, 2016-11-16 at 11:38 -0800, Stephen Boyd wrote: > On 11/16, Arnd Bergmann wrote: > > After the MT2701 clock driver was added, we get a harmless warning for > > the iommu driver that selects it, when compile-testing without > > COMMON_CLK. > > > > warning: (MTK_IOMMU_V1) selects COMMON_CLK_MT2701_IMGSYS which has unmet direct dependencies (COMMON_CLK) > > > > Adding a dependency on COMMON_CLK avoids the warning. > > > > Fixes: e9862118272a ("clk: mediatek: Add MT2701 clock support") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Hm.. why is an iommu driver selecting a clk driver? They should > be using standard clk APIs so it's not like they need it for > build time correctness. Shouldn't we drop the selects instead? > Those look to have been introduced a few kernel versions ago, but > they were selecting options that didn't exist until a few days > ago when I merged the mediatek clk driver. The clk options are > user-visible, so it should be possible to select them in the > configuration phase. > Hi, Stephen, I'm a bit out of date of the current clock code. Mediatek IOMMU v1 driver will need smi driver to enable iommu clients. And smi driver is also respond to enable/disable the susbsys clocks for multi-media HW. The relationship between iommu and smi is like the graphics below[1]. EMI (External Memory Interface) | m4u (Multimedia Memory Management Unit) | SMI Common(Smart Multimedia Interface Common) | +----------------+------- | | | | SMI larb0 SMI larb1 ... SoCs have several SMI local arbiter(larb). (display) (vdec) | | | | +-----+-----+ +----+----+ | | | | | | | | |... | | | ... There are different ports in each larb. | | | | | | OVL0 RDMA0 WDMA0 MC PP VLD When enable SMI driver it will need those subsys clock provider. But those clocks providers are disabled in default. Since it's needed by smi driver, and smi was select by MTK_IOMMU_V1, I figure it should be select by MTK_IOMMU_V1 too. [1]Documentation/devicetree/bindings/iommu/mediatek,iommu.txt thanks. > ----8<---- > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig > index 8ee54d71c7eb..37e204f3d9be 100644 > --- a/drivers/iommu/Kconfig > +++ b/drivers/iommu/Kconfig > @@ -352,9 +352,6 @@ config MTK_IOMMU_V1 > select IOMMU_API > select MEMORY > select MTK_SMI > - select COMMON_CLK_MT2701_MMSYS > - select COMMON_CLK_MT2701_IMGSYS > - select COMMON_CLK_MT2701_VDECSYS > help > Support for the M4U on certain Mediatek SoCs. M4U generation 1 HW is > Multimedia Memory Managememt Unit. This option enables remapping of > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: mtk: add common-clk dependency 2016-11-17 1:25 ` Honghui Zhang @ 2016-11-17 23:35 ` Stephen Boyd 2016-11-18 2:32 ` Honghui Zhang 0 siblings, 1 reply; 5+ messages in thread From: Stephen Boyd @ 2016-11-17 23:35 UTC (permalink / raw) To: Honghui Zhang Cc: Arnd Bergmann, James Liao, linux-kernel, iommu, linux-mediatek, Shunli Wang, Matthias Brugger, Erin Lo, linux-arm-kernel, John Crispin On 11/17, Honghui Zhang wrote: > On Wed, 2016-11-16 at 11:38 -0800, Stephen Boyd wrote: > > On 11/16, Arnd Bergmann wrote: > > > After the MT2701 clock driver was added, we get a harmless warning for > > > the iommu driver that selects it, when compile-testing without > > > COMMON_CLK. > > > > > > warning: (MTK_IOMMU_V1) selects COMMON_CLK_MT2701_IMGSYS which has unmet direct dependencies (COMMON_CLK) > > > > > > Adding a dependency on COMMON_CLK avoids the warning. > > > > > > Fixes: e9862118272a ("clk: mediatek: Add MT2701 clock support") > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > Hm.. why is an iommu driver selecting a clk driver? They should > > be using standard clk APIs so it's not like they need it for > > build time correctness. Shouldn't we drop the selects instead? > > Those look to have been introduced a few kernel versions ago, but > > they were selecting options that didn't exist until a few days > > ago when I merged the mediatek clk driver. The clk options are > > user-visible, so it should be possible to select them in the > > configuration phase. > > > > Hi, Stephen, > I'm a bit out of date of the current clock code. Mediatek IOMMU v1 > driver will need smi driver to enable iommu clients. And smi driver is > also respond to enable/disable the susbsys clocks for multi-media HW. > The relationship between iommu and smi is like the graphics below[1]. > > EMI (External Memory Interface) > | > m4u (Multimedia Memory Management Unit) > | > SMI Common(Smart Multimedia Interface Common) > | > +----------------+------- > | | > | | > SMI larb0 SMI larb1 ... SoCs have several SMI local > arbiter(larb). > (display) (vdec) > | | > | | > +-----+-----+ +----+----+ > | | | | | | > | | |... | | | ... There are different ports in each > larb. > | | | | | | > OVL0 RDMA0 WDMA0 MC PP VLD > > > When enable SMI driver it will need those subsys clock provider. > But those clocks providers are disabled in default. Since it's needed by > smi driver, and smi was select by MTK_IOMMU_V1, I figure it should be > select by MTK_IOMMU_V1 too. Ok I understand all that, but I don't understand why that means we need to have select statements for clk drivers still. If anything, that logic would mean the SMI driver should select clk drivers. I hope it isn't doing that. BTW, I don't understand the mtk_smi_larb_get() API. It looks like we expect the SMI driver to probe and succeed before the mtk_smi_larb_get() function is called. That seems fairly brittle in the face of probe defer or device ordering changes. The SMI driver actually looks like a bus driver for an interconnect as well, but drivers/memory is for memory controllers? Odd but I can get over that. -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] iommu: mtk: add common-clk dependency 2016-11-17 23:35 ` Stephen Boyd @ 2016-11-18 2:32 ` Honghui Zhang 0 siblings, 0 replies; 5+ messages in thread From: Honghui Zhang @ 2016-11-18 2:32 UTC (permalink / raw) To: Stephen Boyd Cc: Arnd Bergmann, James Liao, linux-kernel, iommu, linux-mediatek, Shunli Wang, Matthias Brugger, Erin Lo, linux-arm-kernel, John Crispin On Thu, 2016-11-17 at 15:35 -0800, Stephen Boyd wrote: > On 11/17, Honghui Zhang wrote: > > On Wed, 2016-11-16 at 11:38 -0800, Stephen Boyd wrote: > > > On 11/16, Arnd Bergmann wrote: > > > > After the MT2701 clock driver was added, we get a harmless warning for > > > > the iommu driver that selects it, when compile-testing without > > > > COMMON_CLK. > > > > > > > > warning: (MTK_IOMMU_V1) selects COMMON_CLK_MT2701_IMGSYS which has unmet direct dependencies (COMMON_CLK) > > > > > > > > Adding a dependency on COMMON_CLK avoids the warning. > > > > > > > > Fixes: e9862118272a ("clk: mediatek: Add MT2701 clock support") > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > > > Hm.. why is an iommu driver selecting a clk driver? They should > > > be using standard clk APIs so it's not like they need it for > > > build time correctness. Shouldn't we drop the selects instead? > > > Those look to have been introduced a few kernel versions ago, but > > > they were selecting options that didn't exist until a few days > > > ago when I merged the mediatek clk driver. The clk options are > > > user-visible, so it should be possible to select them in the > > > configuration phase. > > > > > > > Hi, Stephen, > > I'm a bit out of date of the current clock code. Mediatek IOMMU v1 > > driver will need smi driver to enable iommu clients. And smi driver is > > also respond to enable/disable the susbsys clocks for multi-media HW. > > The relationship between iommu and smi is like the graphics below[1]. > > > > EMI (External Memory Interface) > > | > > m4u (Multimedia Memory Management Unit) > > | > > SMI Common(Smart Multimedia Interface Common) > > | > > +----------------+------- > > | | > > | | > > SMI larb0 SMI larb1 ... SoCs have several SMI local > > arbiter(larb). > > (display) (vdec) > > | | > > | | > > +-----+-----+ +----+----+ > > | | | | | | > > | | |... | | | ... There are different ports in each > > larb. > > | | | | | | > > OVL0 RDMA0 WDMA0 MC PP VLD > > > > > > When enable SMI driver it will need those subsys clock provider. > > But those clocks providers are disabled in default. Since it's needed by > > smi driver, and smi was select by MTK_IOMMU_V1, I figure it should be > > select by MTK_IOMMU_V1 too. > > Ok I understand all that, but I don't understand why that means > we need to have select statements for clk drivers still. If > anything, that logic would mean the SMI driver should select clk > drivers. I hope it isn't doing that. > OK, I guess the only reason of "SMI driver select clk driver" is to avoid runtime error. Maybe this is not necessary since runtime errors should be guaranteed by defconfig. Your propose of just remove the select statements is good enough for this problem, thanks. > BTW, I don't understand the mtk_smi_larb_get() API. It looks like > we expect the SMI driver to probe and succeed before the > mtk_smi_larb_get() function is called. That seems fairly brittle > in the face of probe defer or device ordering changes. > Sharp eyes. As a matter of fact, we are struggling on this problem for the moment[1], I guess the recently merged device link's patch may help with this, but I didn't have a change to try that yet. > The SMI driver actually looks like a bus driver for an > interconnect as well, but drivers/memory is for memory > controllers? Odd but I can get over that. > This have been discussed long times ago, seems the current folder is where no one object[2][3]. [1]https://lkml.org/lkml/2016/11/15/232 [2]https://lists.linuxfoundation.org/pipermail/iommu/2015-March/012497.html [3]https://lists.linuxfoundation.org/pipermail/iommu/2015-March/012498.html ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-18 2:32 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-16 15:28 [PATCH] iommu: mtk: add common-clk dependency Arnd Bergmann 2016-11-16 19:38 ` Stephen Boyd 2016-11-17 1:25 ` Honghui Zhang 2016-11-17 23:35 ` Stephen Boyd 2016-11-18 2:32 ` Honghui Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).