Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1291417 > unrolled thread

Re: [PATCH v6 3/5] memory: mediatek: Add SMI driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 3/5] memory: mediatek: Add SMI driver Matthias Brugger <matthias.bgg@gmail.com> - 2015-12-14 19:20 +0100
    Re: [PATCH v6 3/5] memory: mediatek: Add SMI driver Yong Wu <yong.wu@mediatek.com> - 2015-12-15 03:40 +0100
      Re: [PATCH v6 3/5] memory: mediatek: Add SMI driver Daniel Kurtz <djkurtz@chromium.org> - 2015-12-15 06:50 +0100
        Re: [PATCH v6 3/5] memory: mediatek: Add SMI driver Yong Wu <yong.wu@mediatek.com> - 2015-12-16 07:10 +0100

#1291417 — Re: [PATCH v6 3/5] memory: mediatek: Add SMI driver

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

--
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/

[toc] | [next] | [standalone]

#1291795

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




--
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/

[toc] | [prev] | [next] | [standalone]

#1291866

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
--
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/

[toc] | [prev] | [next] | [standalone]

#1292781

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


--
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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web