Re: [PATCH] iommu: mtk: add common-clk dependency

[Stephen Boyd]

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@xxxxxxxx>
> > 
> > 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