Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751809AbbLOFpt (ORCPT ); Tue, 15 Dec 2015 00:45:49 -0500 Received: from mail-vk0-f49.google.com ([209.85.213.49]:33665 "EHLO mail-vk0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750861AbbLOFpr (ORCPT ); Tue, 15 Dec 2015 00:45:47 -0500 MIME-Version: 1.0 In-Reply-To: <1450147103.22854.23.camel@mhfsdcap03> References: <1449568153-15643-1-git-send-email-yong.wu@mediatek.com> <1449568153-15643-4-git-send-email-yong.wu@mediatek.com> <24171857.0SPpBlzoZl@linux-gy6r.site> <1450147103.22854.23.camel@mhfsdcap03> From: Daniel Kurtz Date: Tue, 15 Dec 2015 13:45:26 +0800 X-Google-Sender-Auth: 31uZlMNB6YSiH_XN4zKfWzksTdo Message-ID: Subject: Re: [PATCH v6 3/5] memory: mediatek: Add SMI driver To: Yong Wu Cc: Matthias Brugger , Joerg Roedel , Thierry Reding , Mark Rutland , Robin Murphy , Will Deacon , Tomasz Figa , Lucas Stach , Rob Herring , Catalin Marinas , linux-mediatek@lists.infradead.org, Sasha Hauer , srv_heupstream , "open list:OPEN FIRMWARE AND..." , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "open list:IOMMU DRIVERS" , Paul Bolle , Arnd Bergmann , mitchelh@codeaurora.org, Philipp Zabel , Yingjoe Chen Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6546 Lines: 217 Hi Yong, On Tue, Dec 15, 2015 at 10:38 AM, Yong Wu wrote: > On Mon, 2015-12-14 at 19:18 +0100, Matthias Brugger wrote: >> On Tuesday 08 Dec 2015 17:49:11 Yong Wu wrote: >> > This patch add SMI(Smart Multimedia Interface) driver. This driver >> > is responsible to enable/disable iommu and control the power domain >> > and clocks of each local arbiter. >> > >> > Signed-off-by: Yong Wu >> > --- >> > Currently SMI offer mtk_smi_larb_get/put to enable the power-domain >> > ,clocks and initialize the iommu configuration register for each a local >> > arbiter, The reason is: >> > a) If a device would like to disable iommu, it also need call >> > mtk_smi_larb_get/put to enable its power and clocks. >> > b) The iommu core don't support attach/detach a device within a >> > iommu-group. So we cann't use iommu_attach_device(iommu_detach_device) >> > instead >> > of mtk_smi_larb_get/put. >> > > [..] >> > +static int >> > +mtk_smi_enable(struct device *dev, struct clk *apb, struct clk *smi) >> > +{ >> > + int ret; >> > + >> > + ret = pm_runtime_get_sync(dev); >> > + if (ret < 0) >> > + return ret; >> > + >> > + ret = clk_prepare_enable(apb); >> > + if (ret) >> > + goto err_put_pm; >> > + >> > + ret = clk_prepare_enable(smi); >> > + if (ret) >> > + goto err_disable_apb; >> > + >> > + return 0; >> > + >> > +err_disable_apb: >> > + clk_disable_unprepare(apb); >> > +err_put_pm: >> > + pm_runtime_put_sync(dev); >> > + return ret; >> > +} >> > + >> > +static void >> > +mtk_smi_disable(struct device *dev, struct clk *apb, struct clk *smi) >> > +{ >> > + clk_disable_unprepare(smi); >> > + clk_disable_unprepare(apb); >> > + pm_runtime_put_sync(dev); >> > +} >> > + >> > +static int mtk_smi_common_enable(struct mtk_smi_common *common) >> > +{ >> > + return mtk_smi_enable(common->dev, common->clk_apb, common->clk_smi); >> > +} >> > + >> > +static void mtk_smi_common_disable(struct mtk_smi_common *common) >> > +{ >> > + mtk_smi_disable(common->dev, common->clk_apb, common->clk_smi); >> > +} >> > + >> > +static int mtk_smi_larb_enable(struct mtk_smi_larb *larb) >> > +{ >> > + return mtk_smi_enable(larb->dev, larb->clk_apb, larb->clk_smi); >> > +} >> > + >> > +static void mtk_smi_larb_disable(struct mtk_smi_larb *larb) >> > +{ >> > + mtk_smi_disable(larb->dev, larb->clk_apb, larb->clk_smi); >> > +} >> > + >> >> This is somehow over-engineered. Just use mtk_smi_enable and mtk_smi_disable >> instead of adding an extra indirection. > > I added this only for readable...then the code in mtk_smi_larb_get below > may looks simple and readable. > > If I use mtk_smi_enable/disable directly, the code will be like our > v5[1], is it OK? > Maybe I don't need these help function here, and only add more comment > based on v5. > > [1] > http://lists.linuxfoundation.org/pipermail/iommu/2015-October/014590.html bike-shedding... I like the fact that Yong is trying to make his helpers more type-safe. But, perhaps we can rename "struct mtk_smi_common" as "struct mtk_smi", and then make "struct mtk_smi_larb" contain a "struct mtk_smi": struct mtk_smi { struct device *dev; struct clk *clk_apb, *clk_smi; } struct mtk_smi_larb { struct mtk_smi; ... } Then, have: int mtk_smi_enable(struct mtk_smi *smi) { clk_enable(smi->clk_apb); ... } int mtk_smi_disable(struct mtk_smi *smi) { } int mtk_smi_larb_get(struct device *larbdev) { struct mtk_smi_larb *larb = dev_get_drvdata(larbdev); struct mtk_smi *common = dev_get_drvdata(larb->smi_common_dev); mtk_smi_enable(common); mtk_smi_enable(&larb->smi); ... } >> >> > +int mtk_smi_larb_get(struct device *larbdev) >> > +{ >> > + struct mtk_smi_larb *larb = dev_get_drvdata(larbdev); >> > + struct mtk_smi_common *common = dev_get_drvdata(larb->smi_common_dev); >> > + int ret; >> > + >> > + ret = mtk_smi_common_enable(common); >> > + if (ret) >> > + return ret; >> > + >> > + ret = mtk_smi_larb_enable(larb); >> > + if (ret) >> > + goto err_put_smi; >> > + >> > + /* Configure the iommu info */ >> > + writel_relaxed(larb->mmu, larb->base + SMI_LARB_MMU_EN); I think this should probably be writel() not writel_relaxed, since you really do want the barrier to ensure all other register accesses have completed before enabling the MMU. >> > + >> > + return 0; >> > + >> > +err_put_smi: >> > + mtk_smi_common_disable(common); >> > + return ret; >> > +} >> > + >> > +void mtk_smi_larb_put(struct device *larbdev) >> > +{ >> > + struct mtk_smi_larb *larb = dev_get_drvdata(larbdev); >> > + struct mtk_smi_common *common = dev_get_drvdata(larb->smi_common_dev); >> > + >> > + writel_relaxed(0, larb->base + SMI_LARB_MMU_EN); >> > + mtk_smi_larb_disable(larb); >> > + mtk_smi_common_disable(common); >> > +} >> > + >> >> Looks strange that you just disable all MMUs while you only enable some of >> them at runtime. Unfortunately the datasheet I have lacks the SMI part, so I >> can just guess how the HW is working. >> From the DTS it looks like as if a larb can be used by two different >> components (e.g. larb0 from ovl0 and rdma0). Wouldn't that produce a conflict? > > Thanks. It's really a problem. > > There are OVL0 and MDP in larb0, Both will call mtk_smi_larb_get/put, we > cann't disable all the MMUs in whole the larb0 here. This register > should be reset to zero while the larb power domain turning off(rely on > the power-domain ref count). > I will delete this(keep this in our V5.) Hmm, mtk_smi_config_port(.., false) clears the bit in larb->mmu, but does not actually "disable" an enabled mmu. The MMU will be disabled only on the next mtk_smi_larb_get() (for a different port on the same larb). I guess this is ok. The only weird thing is this situation, where an MMU can be left enabled when its user is done with it: /* configure port 0 as 'enabled' */ mtk_smi_config_port(0, true); /* configure port 1 as 'enabled' */ mtk_smi_config_port(1, true); /* user of port 0 wants to do work */ mtk_smi_larb_get() /* turns on all clks, power & enables both MMUs */ /* user of port 1 wants to do work */ mtk_smi_larb_get() /* user of port 1 done doing work */ mtk_smi_larb_put() /* MMU 1 is still enabled */ Thanks! -Dan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/