| Message ID | 1449568153-15643-4-git-send-email-yong.wu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2015-12-08 at 17:49 +0800, 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 <yong.wu@mediatek.com> > --- Hi Matthias, Because drivers/memory/ don't have the maintainer, and our IOMMU, V4L2 and DRM rely on the SMI. From Joerg and Thierry[1], we need your help. Could you have a loot at our SMI while free? Look forward to any comment from you. Thanks. [1]http://lists.linuxfoundation.org/pipermail/iommu/2015-November/014981.html
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 <yong.wu@mediatek.com> > --- > 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. > > drivers/memory/Kconfig | 8 ++ > drivers/memory/Makefile | 1 + > drivers/memory/mtk-smi.c | 297 > +++++++++++++++++++++++++++++++++++++++++++++ include/soc/mediatek/smi.h | > 53 ++++++++ > 4 files changed, 359 insertions(+) > create mode 100644 drivers/memory/mtk-smi.c > create mode 100644 include/soc/mediatek/smi.h > > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig > index 6f31546..51d5cd2 100644 > --- a/drivers/memory/Kconfig > +++ b/drivers/memory/Kconfig > @@ -114,6 +114,14 @@ config JZ4780_NEMC > the Ingenic JZ4780. This controller is used to handle external > memory devices such as NAND and SRAM. > > +config MTK_SMI > + bool > + depends on ARCH_MEDIATEK || COMPILE_TEST > + help > + This driver is for the Memory Controller module in MediaTek SoCs, > + mainly help enable/disable iommu and control the power domain and > + clocks for each local arbiter. > + > source "drivers/memory/tegra/Kconfig" > > endif > diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile > index 1c46af5..890bdf4 100644 > --- a/drivers/memory/Makefile > +++ b/drivers/memory/Makefile > @@ -15,5 +15,6 @@ obj-$(CONFIG_FSL_IFC) += fsl_ifc.o > obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o > obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o > obj-$(CONFIG_JZ4780_NEMC) += jz4780-nemc.o > +obj-$(CONFIG_MTK_SMI) += mtk-smi.o > > obj-$(CONFIG_TEGRA_MC) += tegra/ > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c > new file mode 100644 > index 0000000..3562081 > --- /dev/null > +++ b/drivers/memory/mtk-smi.c > @@ -0,0 +1,297 @@ > +/* > + * Copyright (c) 2014-2015 MediaTek Inc. > + * Author: Yong Wu <yong.wu@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#include <linux/clk.h> > +#include <linux/component.h> > +#include <linux/err.h> > +#include <linux/io.h> > +#include <linux/of_address.h> > +#include <linux/of_platform.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <soc/mediatek/smi.h> > + > +#define SMI_LARB_MMU_EN 0xf00 > +#define F_SMI_MMU_EN(port) BIT(port) > + > +struct mtk_smi_common { > + struct device *dev; > + struct clk *clk_apb, *clk_smi; > +}; > + > +struct mtk_smi_larb { /* larb: local arbiter */ > + struct device *dev; > + void __iomem *base; > + struct clk *clk_apb, *clk_smi; > + struct device *smi_common_dev; > + u32 mmu; > +}; > + > +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. > +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); > + > + 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? Regards, Matthias > +void mtk_smi_config_port(struct device *larbdev, unsigned int larbportid, > + bool enable) > +{ > + struct mtk_smi_larb *larb = dev_get_drvdata(larbdev); > + > + dev_dbg(larbdev, "%s iommu port: %d\n", > + enable ? "enable" : "disable", larbportid); > + > + /* > + * Only record the iommu info here, > + * and it will work after its power and clocks is enabled. > + */ > + if (enable) > + larb->mmu |= F_SMI_MMU_EN(larbportid); > + else > + larb->mmu &= ~F_SMI_MMU_EN(larbportid); > +} > + > +static int > +mtk_smi_larb_bind(struct device *dev, struct device *master, void *data) > +{ > + return 0; > +} > + > +static void > +mtk_smi_larb_unbind(struct device *dev, struct device *master, void *data) > +{ > +} > + > +static const struct component_ops mtk_smi_larb_component_ops = { > + .bind = mtk_smi_larb_bind, > + .unbind = mtk_smi_larb_unbind, > +}; > + > +static int mtk_smi_larb_probe(struct platform_device *pdev) > +{ > + struct mtk_smi_larb *larb; > + struct resource *res; > + struct device *dev = &pdev->dev; > + struct device_node *smi_node; > + struct platform_device *smi_pdev; > + > + if (!dev->pm_domain) > + return -EPROBE_DEFER; > + > + larb = devm_kzalloc(dev, sizeof(*larb), GFP_KERNEL); > + if (!larb) > + return -ENOMEM; > + larb->dev = dev; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + larb->base = devm_ioremap_resource(dev, res); > + if (IS_ERR(larb->base)) > + return PTR_ERR(larb->base); > + > + larb->clk_apb = devm_clk_get(dev, "apb"); > + if (IS_ERR(larb->clk_apb)) > + return PTR_ERR(larb->clk_apb); > + > + larb->clk_smi = devm_clk_get(dev, "smi"); > + if (IS_ERR(larb->clk_smi)) > + return PTR_ERR(larb->clk_smi); > + > + smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0); > + if (!smi_node) > + return -EINVAL; > + > + smi_pdev = of_find_device_by_node(smi_node); > + of_node_put(smi_node); > + if (smi_pdev) { > + larb->smi_common_dev = &smi_pdev->dev; > + } else { > + dev_err(dev, "Failed to get the smi_common device\n"); > + return -EINVAL; > + } > + > + pm_runtime_enable(dev); > + dev_set_drvdata(dev, larb); > + return component_add(dev, &mtk_smi_larb_component_ops); > +} > + > +static int mtk_smi_larb_remove(struct platform_device *pdev) > +{ > + pm_runtime_disable(&pdev->dev); > + component_del(&pdev->dev, &mtk_smi_larb_component_ops); > + return 0; > +} > + > +static const struct of_device_id mtk_smi_larb_of_ids[] = { > + { .compatible = "mediatek,mt8173-smi-larb",}, > + {} > +}; > + > +static struct platform_driver mtk_smi_larb_driver = { > + .probe = mtk_smi_larb_probe, > + .remove = mtk_smi_larb_remove, > + .driver = { > + .name = "mtk-smi-larb", > + .of_match_table = mtk_smi_larb_of_ids, > + } > +}; > + > +static int mtk_smi_common_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct mtk_smi_common *common; > + > + if (!dev->pm_domain) > + return -EPROBE_DEFER; > + > + common = devm_kzalloc(dev, sizeof(*common), GFP_KERNEL); > + if (!common) > + return -ENOMEM; > + common->dev = dev; > + > + common->clk_apb = devm_clk_get(dev, "apb"); > + if (IS_ERR(common->clk_apb)) > + return PTR_ERR(common->clk_apb); > + > + common->clk_smi = devm_clk_get(dev, "smi"); > + if (IS_ERR(common->clk_smi)) > + return PTR_ERR(common->clk_smi); > + > + pm_runtime_enable(dev); > + dev_set_drvdata(dev, common); > + return 0; > +} > + > +static int mtk_smi_common_remove(struct platform_device *pdev) > +{ > + pm_runtime_disable(&pdev->dev); > + return 0; > +} > + > +static const struct of_device_id mtk_smi_common_of_ids[] = { > + { .compatible = "mediatek,mt8173-smi-common", }, > + {} > +}; > + > +static struct platform_driver mtk_smi_common_driver = { > + .probe = mtk_smi_common_probe, > + .remove = mtk_smi_common_remove, > + .driver = { > + .name = "mtk-smi-common", > + .of_match_table = mtk_smi_common_of_ids, > + } > +}; > + > +static int __init mtk_smi_init(void) > +{ > + int ret; > + > + ret = platform_driver_register(&mtk_smi_common_driver); > + if (ret != 0) { > + pr_err("Failed to register SMI driver\n"); > + return ret; > + } > + > + ret = platform_driver_register(&mtk_smi_larb_driver); > + if (ret != 0) { > + pr_err("Failed to register SMI-LARB driver\n"); > + goto err_unreg_smi; > + } > + return ret; > + > +err_unreg_smi: > + platform_driver_unregister(&mtk_smi_common_driver); > + return ret; > +} > +subsys_initcall(mtk_smi_init); > diff --git a/include/soc/mediatek/smi.h b/include/soc/mediatek/smi.h > new file mode 100644 > index 0000000..6f41b2e > --- /dev/null > +++ b/include/soc/mediatek/smi.h > @@ -0,0 +1,53 @@ > +/* > + * Copyright (c) 2014-2015 MediaTek Inc. > + * Author: Yong Wu <yong.wu@mediatek.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > +#ifndef MTK_IOMMU_SMI_H > +#define MTK_IOMMU_SMI_H > + > +#include <linux/device.h> > + > +#ifdef CONFIG_MTK_SMI > + > +/* > + * Record the iommu info for each port in the local arbiter. > + * It's only for iommu. > + */ > +void mtk_smi_config_port(struct device *larbdev, unsigned int larbportid, > + bool enable); > +/* > + * mtk_smi_larb_get: Enable the power domain and clocks for this local > arbiter. + * It also initialize some basic setting(like > iommu). + * mtk_smi_larb_put: Disable the power domain and clocks for this > local arbiter. + * Both should be called in non-atomic context. > + * > + * Returns 0 if successful, negative on failure. > + */ > +int mtk_smi_larb_get(struct device *larbdev); > +void mtk_smi_larb_put(struct device *larbdev); > + > +#else > + > +static inline void > +mtk_smi_config_port(struct device *larbdev, unsigned int larbportid, > + bool enable) { } > + > +static inline int mtk_smi_larb_get(struct device *larbdev) > +{ > + return 0; > +} > + > +static inline void mtk_smi_larb_put(struct device *larbdev) { } > + > +#endif > + > +#endif
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 <yong.wu@mediatek.com> > > --- > > 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 > > > +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); > > + > > + 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.) > > Regards, > Matthias
Hi Yong, On Tue, Dec 15, 2015 at 10:38 AM, Yong Wu <yong.wu@mediatek.com> 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 <yong.wu@mediatek.com> >> > --- >> > 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
On Tue, 2015-12-15 at 13:45 +0800, Daniel Kurtz wrote: > Hi Yong, > > On Tue, Dec 15, 2015 at 10:38 AM, Yong Wu <yong.wu@mediatek.com> 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 <yong.wu@mediatek.com> > >> > --- > >> > 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); > ... > } Thanks. I will change like this in next time. > > >> > >> > +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. Yes. I will fix this. > > >> > + > >> > + 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. Actually mtk_smi_config_port(.., false) will never be called currently. If anybody would like to call iommu_detach_device to config-port false. He will get the log below since the current iommu core don't support detach one device in a iommu-group which have many devices. That's to say that the larb->mmu is initialized in probe, and will never be changed again. (151119_13:39:37.472)WARNING: at /proj/mtk40525/upstreamdev/v4.4/kernel/mediatek/drivers/iommu/iommu.c:1154 (151119_13:39:37.472)Modules linked in: (151119_13:39:37.472)CPU: 1 PID: 731 Comm: sh Not tainted 4.4.0-rc1+ #37 (151119_13:39:37.472)Hardware name: MediaTek MT8173 evaluation board (DT) (151119_13:39:37.472)task: ffffffc076bb4d00 ti: ffffffc076bdc000 task.ti: ffffffc076bdc000 (151119_13:39:37.472)PC is at iommu_detach_device+0x5c/0xb0 (151119_13:39:37.472)LR is at iommu_detach_device+0x30/0xb0 ... (151119_13:39:37.550)---[ end trace d831cba9f811edf3 ]--- (151119_13:39:37.550)Call trace: (151119_13:39:37.550)[<ffffffc0003f8c70>] iommu_detach_device+0x5c/0xb0 By the way, In the next version I plan to delete this interface mtk_smi_config_port, use the additional data of the component-bind instead of this. > 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 */ This is really not so perfect in this case. Even though the iommu-core support iommu-detach dynamically for our case in the future(there is no plan currently), This don't have bad effect. If user of port 1 want to work again, He should call mtk_smi_larb_get again. From the HW, there may be different modules in a local arbiter, SMI here can not detect which module call mtk_smi_larb_put and disable his special iommu bits. After all mtk_smi_larb_put is mainly for power off and disable the clocks. > > > Thanks! > -Dan
diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig index 6f31546..51d5cd2 100644 --- a/drivers/memory/Kconfig +++ b/drivers/memory/Kconfig @@ -114,6 +114,14 @@ config JZ4780_NEMC the Ingenic JZ4780. This controller is used to handle external memory devices such as NAND and SRAM. +config MTK_SMI + bool + depends on ARCH_MEDIATEK || COMPILE_TEST + help + This driver is for the Memory Controller module in MediaTek SoCs, + mainly help enable/disable iommu and control the power domain and + clocks for each local arbiter. + source "drivers/memory/tegra/Kconfig" endif diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile index 1c46af5..890bdf4 100644 --- a/drivers/memory/Makefile +++ b/drivers/memory/Makefile @@ -15,5 +15,6 @@ obj-$(CONFIG_FSL_IFC) += fsl_ifc.o obj-$(CONFIG_MVEBU_DEVBUS) += mvebu-devbus.o obj-$(CONFIG_TEGRA20_MC) += tegra20-mc.o obj-$(CONFIG_JZ4780_NEMC) += jz4780-nemc.o +obj-$(CONFIG_MTK_SMI) += mtk-smi.o obj-$(CONFIG_TEGRA_MC) += tegra/ diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c new file mode 100644 index 0000000..3562081 --- /dev/null +++ b/drivers/memory/mtk-smi.c @@ -0,0 +1,297 @@ +/* + * Copyright (c) 2014-2015 MediaTek Inc. + * Author: Yong Wu <yong.wu@mediatek.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include <linux/clk.h> +#include <linux/component.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <soc/mediatek/smi.h> + +#define SMI_LARB_MMU_EN 0xf00 +#define F_SMI_MMU_EN(port) BIT(port) + +struct mtk_smi_common { + struct device *dev; + struct clk *clk_apb, *clk_smi; +}; + +struct mtk_smi_larb { /* larb: local arbiter */ + struct device *dev; + void __iomem *base; + struct clk *clk_apb, *clk_smi; + struct device *smi_common_dev; + u32 mmu; +}; + +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); +} + +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); + + 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); +} + +void mtk_smi_config_port(struct device *larbdev, unsigned int larbportid, + bool enable) +{ + struct mtk_smi_larb *larb = dev_get_drvdata(larbdev); + + dev_dbg(larbdev, "%s iommu port: %d\n", + enable ? "enable" : "disable", larbportid); + + /* + * Only record the iommu info here, + * and it will work after its power and clocks is enabled. + */ + if (enable) + larb->mmu |= F_SMI_MMU_EN(larbportid); + else + larb->mmu &= ~F_SMI_MMU_EN(larbportid); +} + +static int +mtk_smi_larb_bind(struct device *dev, struct device *master, void *data) +{ + return 0; +} + +static void +mtk_smi_larb_unbind(struct device *dev, struct device *master, void *data) +{ +} + +static const struct component_ops mtk_smi_larb_component_ops = { + .bind = mtk_smi_larb_bind, + .unbind = mtk_smi_larb_unbind, +}; + +static int mtk_smi_larb_probe(struct platform_device *pdev) +{ + struct mtk_smi_larb *larb; + struct resource *res; + struct device *dev = &pdev->dev; + struct device_node *smi_node; + struct platform_device *smi_pdev; + + if (!dev->pm_domain) + return -EPROBE_DEFER; + + larb = devm_kzalloc(dev, sizeof(*larb), GFP_KERNEL); + if (!larb) + return -ENOMEM; + larb->dev = dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + larb->base = devm_ioremap_resource(dev, res); + if (IS_ERR(larb->base)) + return PTR_ERR(larb->base); + + larb->clk_apb = devm_clk_get(dev, "apb"); + if (IS_ERR(larb->clk_apb)) + return PTR_ERR(larb->clk_apb); + + larb->clk_smi = devm_clk_get(dev, "smi"); + if (IS_ERR(larb->clk_smi)) + return PTR_ERR(larb->clk_smi); + + smi_node = of_parse_phandle(dev->of_node, "mediatek,smi", 0); + if (!smi_node) + return -EINVAL; + + smi_pdev = of_find_device_by_node(smi_node); + of_node_put(smi_node); + if (smi_pdev) { + larb->smi_common_dev = &smi_pdev->dev; + } else { + dev_err(dev, "Failed to get the smi_common device\n"); + return -EINVAL; + } + + pm_runtime_enable(dev); + dev_set_drvdata(dev, larb); + return component_add(dev, &mtk_smi_larb_component_ops); +} + +static int mtk_smi_larb_remove(struct platform_device *pdev) +{ + pm_runtime_disable(&pdev->dev); + component_del(&pdev->dev, &mtk_smi_larb_component_ops); + return 0; +} + +static const struct of_device_id mtk_smi_larb_of_ids[] = { + { .compatible = "mediatek,mt8173-smi-larb",}, + {} +}; + +static struct platform_driver mtk_smi_larb_driver = { + .probe = mtk_smi_larb_probe, + .remove = mtk_smi_larb_remove, + .driver = { + .name = "mtk-smi-larb", + .of_match_table = mtk_smi_larb_of_ids, + } +}; + +static int mtk_smi_common_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct mtk_smi_common *common; + + if (!dev->pm_domain) + return -EPROBE_DEFER; + + common = devm_kzalloc(dev, sizeof(*common), GFP_KERNEL); + if (!common) + return -ENOMEM; + common->dev = dev; + + common->clk_apb = devm_clk_get(dev, "apb"); + if (IS_ERR(common->clk_apb)) + return PTR_ERR(common->clk_apb); + + common->clk_smi = devm_clk_get(dev, "smi"); + if (IS_ERR(common->clk_smi)) + return PTR_ERR(common->clk_smi); + + pm_runtime_enable(dev); + dev_set_drvdata(dev, common); + return 0; +} + +static int mtk_smi_common_remove(struct platform_device *pdev) +{ + pm_runtime_disable(&pdev->dev); + return 0; +} + +static const struct of_device_id mtk_smi_common_of_ids[] = { + { .compatible = "mediatek,mt8173-smi-common", }, + {} +}; + +static struct platform_driver mtk_smi_common_driver = { + .probe = mtk_smi_common_probe, + .remove = mtk_smi_common_remove, + .driver = { + .name = "mtk-smi-common", + .of_match_table = mtk_smi_common_of_ids, + } +}; + +static int __init mtk_smi_init(void) +{ + int ret; + + ret = platform_driver_register(&mtk_smi_common_driver); + if (ret != 0) { + pr_err("Failed to register SMI driver\n"); + return ret; + } + + ret = platform_driver_register(&mtk_smi_larb_driver); + if (ret != 0) { + pr_err("Failed to register SMI-LARB driver\n"); + goto err_unreg_smi; + } + return ret; + +err_unreg_smi: + platform_driver_unregister(&mtk_smi_common_driver); + return ret; +} +subsys_initcall(mtk_smi_init); diff --git a/include/soc/mediatek/smi.h b/include/soc/mediatek/smi.h new file mode 100644 index 0000000..6f41b2e --- /dev/null +++ b/include/soc/mediatek/smi.h @@ -0,0 +1,53 @@ +/* + * Copyright (c) 2014-2015 MediaTek Inc. + * Author: Yong Wu <yong.wu@mediatek.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#ifndef MTK_IOMMU_SMI_H +#define MTK_IOMMU_SMI_H + +#include <linux/device.h> + +#ifdef CONFIG_MTK_SMI + +/* + * Record the iommu info for each port in the local arbiter. + * It's only for iommu. + */ +void mtk_smi_config_port(struct device *larbdev, unsigned int larbportid, + bool enable); +/* + * mtk_smi_larb_get: Enable the power domain and clocks for this local arbiter. + * It also initialize some basic setting(like iommu). + * mtk_smi_larb_put: Disable the power domain and clocks for this local arbiter. + * Both should be called in non-atomic context. + * + * Returns 0 if successful, negative on failure. + */ +int mtk_smi_larb_get(struct device *larbdev); +void mtk_smi_larb_put(struct device *larbdev); + +#else + +static inline void +mtk_smi_config_port(struct device *larbdev, unsigned int larbportid, + bool enable) { } + +static inline int mtk_smi_larb_get(struct device *larbdev) +{ + return 0; +} + +static inline void mtk_smi_larb_put(struct device *larbdev) { } + +#endif + +#endif
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 <yong.wu@mediatek.com> --- 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. drivers/memory/Kconfig | 8 ++ drivers/memory/Makefile | 1 + drivers/memory/mtk-smi.c | 297 +++++++++++++++++++++++++++++++++++++++++++++ include/soc/mediatek/smi.h | 53 ++++++++ 4 files changed, 359 insertions(+) create mode 100644 drivers/memory/mtk-smi.c create mode 100644 include/soc/mediatek/smi.h