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

[Yong Wu]

On Mon, 2016-01-18 at 11:11 +0100, Matthias Brugger wrote:
> 
> On 18/12/15 09:09, 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 

[...]

> > +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);
> > +   int ret;
> > +
> > +   /* Enable the smi-common's power and clocks */
> > +   ret = mtk_smi_enable(common);
> > +   if (ret)
> > +   return ret;
> > +
> > +   /* Enable the larb's power and clocks */
> > +   ret = mtk_smi_enable(>smi);
> > +   if (ret) {
> > +   mtk_smi_disable(common);
> > +   return ret;
> > +   }
> > +
> > +   /* Configure the iommu info */
> > +   if (larb->mmu)
> > +   writel(*larb->mmu, larb->base + SMI_LARB_MMU_EN);
> 
> Can there be a larb that does not have a mmu id defined?
> Phandles for the iommu need always to have a mtk_m4u_id defined, so I 
> don't see the case where this can happen?

  No. In current IC(mt8173), All the larbs have the mmu info register.

> Why did you changed this value to a pointer and added the if?

  I changed it to a pointer in order to support
iommu_attach_device/iommu_detach_device dynamically.

  In our v6, there is a interface called mtk_smi_config_port in which
the iommu(M4U) could tell SMI which ports should enable iommu.
  In this version, we use the additional data of component to finish
this job(the third parameter of the mtk_smi_larb_bind). then we could
delete that interface.

  larb->mmu is a pointer that point to the mmu in "struct mtk_smi_iommu
smi_imu" of "struct mtk_iommu_data". This "smi_imu" is used to record
that which ports has already enabled iommu for each larb and it will be
updated during the iommu_attach_device/iommu_detach_device.
 
   Why add the if?
  -> This pointer may be null in the mtk_smi_larb_unbind.
  I'm also not sure when it will enter mtk_smi_larb_unbind, maybe while
the iommu device is removed, then it call component_master_del, or the
larb device itself is removed, or some other error case in component.
but there really is a path it will be null, so I add this *if*.
   If you think it is unnecessary, I can delete it.

> 
> > +
> > +   return 0;
> > +}
> > +
> > +void mtk_smi_larb_put(struct device *larbdev)
> > +{
> > +   struct mtk_smi_larb *larb = dev_get_drvdata(larbdev);
> > +   struct mtk_smi *common = dev_get_drvdata(larb->smi_common_dev);
> > +
> 
> Would it make sense to write SMI_LARB_MMU_EN here again to eventually 
> turn off mmus?

I think no. As you know, there may be some modules in one larb, so we
cann't write 0 in SMI_LARB_MMU_EN here to turn off mmus for all modules.

mtk_smi_larb_get/mtk_smi_larb_put mainly enable/disable the power domain
and clocks for this local arbiter. And the larb is a unit here, SMI
cann’t detect which module calls mtk_smi_larb_put and disable his
special iommu bits. so do mtk_smi_larb_get.
 
I know that the module’s special iommu bit wasn’t wrote to 0 here, but
if this module would like to work again, he have to call
mtk_smi_larb_get again, and currently all the multimedia modules always
works via iommu, we don’t need to write 0 here.


> Other then this, the driver looks fine to me.

Thanks very much for your reply. I could send the new version if we get
a conclusion for the two issue above here, like you really don't like
that *if*.

> 
> Regards,
> Matthias
> 
> > +   mtk_smi_disable(>smi);
> > +   mtk_smi_disable(common);
> > +}
> > +
> > +static int
> > +mtk_smi_larb_bind(struct device *dev, struct device *master, void *data)
> > +{
> > +   struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> > +   struct mtk_smi_iommu *smi_iommu = data;
> > +   unsigned int i;
> > +
> > +   for (i = 0; i < smi_iommu->larb_nr; i++) {
> > +   if (dev == smi_iommu->larb_imu[i].dev) {
> > +   larb->mmu = _iommu->larb_imu[i].mmu;
> > +   return 0;
> > +   }
> > +   }
> > +   return -ENODEV;
> > +}
> > +
> > +static void
> > +mtk_smi_larb_unbind(struct device *dev, struct device *master, void *data)
> > +{
> > +   struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> > +
> > +   larb->mmu = NULL;
> > +}
> > +
> > +static const struct component_ops mtk_smi_larb_component_ops = {
> > +   .bind = mtk_smi_larb_bind,
> > +   .unbind = mtk_smi_larb_unbind,
> > +};

[...]


___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Matthias Brugger]

On 18/12/15 09:09, 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 
---
  drivers/memory/Kconfig |   8 ++
  drivers/memory/Makefile|   1 +
  drivers/memory/mtk-smi.c   | 268 +
  include/soc/mediatek/smi.h |  58 ++
  4 files changed, 335 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 000..3714f604
--- /dev/null
+++ b/drivers/memory/mtk-smi.c
@@ -0,0 +1,268 @@
+/*
+ * Copyright (c) 2014-2015 MediaTek Inc.
+ * Author: Yong Wu 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SMI_LARB_MMU_EN0xf00
+
+struct mtk_smi {
+   struct device   *dev;
+   struct clk  *clk_apb, *clk_smi;
+};
+
+struct mtk_smi_larb { /* larb: local arbiter */
+   struct mtk_smi  smi;
+   void __iomem*base;
+   struct device   *smi_common_dev;
+   u32 *mmu;
+};
+
+static int mtk_smi_enable(const struct mtk_smi *smi)
+{
+   int ret;
+
+   ret = pm_runtime_get_sync(smi->dev);
+   if (ret < 0)
+   return ret;
+
+   ret = clk_prepare_enable(smi->clk_apb);
+   if (ret)
+   goto err_put_pm;
+
+   ret = clk_prepare_enable(smi->clk_smi);
+   if (ret)
+   goto err_disable_apb;
+
+   return 0;
+
+err_disable_apb:
+   clk_disable_unprepare(smi->clk_apb);
+err_put_pm:
+   pm_runtime_put_sync(smi->dev);
+   return ret;
+}
+
+static void mtk_smi_disable(const struct mtk_smi *smi)
+{
+   clk_disable_unprepare(smi->clk_smi);
+   clk_disable_unprepare(smi->clk_apb);
+   pm_runtime_put_sync(smi->dev);
+}
+
+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);
+   int ret;
+
+   /* Enable the smi-common's power and clocks */
+   ret = mtk_smi_enable(common);
+   if (ret)
+   return ret;
+
+   /* Enable the larb's power and clocks */
+   ret = mtk_smi_enable(>smi);
+   if (ret) {
+   mtk_smi_disable(common);
+   return ret;
+   }
+
+   /* Configure the iommu info */
+   if (larb->mmu)
+   writel(*larb->mmu, larb->base + SMI_LARB_MMU_EN);


Can there be a larb that does not have a mmu id defined?
Phandles for the iommu need always to have a mtk_m4u_id defined, so I 
don't see the case where this can happen?


Why did you changed this value to a pointer and added the if?


+
+   return 0;
+}
+
+void mtk_smi_larb_put(struct device *larbdev)
+{
+   struct mtk_smi_larb *larb = dev_get_drvdata(larbdev);
+   struct mtk_smi *common = dev_get_drvdata(larb->smi_common_dev);
+


Would it make sense to write SMI_LARB_MMU_EN here again to eventually 
turn off mmus?


Other then this, the driver looks fine to me.

Regards,
Matthias


+   mtk_smi_disable(>smi);
+   mtk_smi_disable(common);
+}
+
+static int
+mtk_smi_larb_bind(struct device *dev, struct device *master, void *data)
+{
+  

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

[Philipp Zabel]

Hi,

Am Montag, den 04.01.2016, 14:56 +0800 schrieb Yong Wu:
> On Fri, 2015-12-18 at 16:09 +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.
> 
> Hi Matthias,
> What's your opinion about the SMI in this version?
> Sorry to disturb you again since SMI need your review before Joerg
> accept it.

How is this going to be merged, eventually? If it could be put on a
separate branch, that would allow me to also include it in the DRM pull
request that will build-depend on the SMI driver.

> > Signed-off-by: Yong Wu 

Tested-by: Philipp Zabel 

regards
Philipp

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Yong Wu]

On Fri, 2015-12-18 at 16:09 +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.

Hi Matthias,
What's your opinion about the SMI in this version?
Sorry to disturb you again since SMI need your review before Joerg
accept it.

> 
> Signed-off-by: Yong Wu 
> ---
>  drivers/memory/Kconfig |   8 ++
>  drivers/memory/Makefile|   1 +
>  drivers/memory/mtk-smi.c   | 268 
> +
>  include/soc/mediatek/smi.h |  58 ++
>  4 files changed, 335 insertions(+)
>  create mode 100644 drivers/memory/mtk-smi.c
>  create mode 100644 include/soc/mediatek/smi.h
> 
[...]

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

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

[Yong Wu]

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 
---
 drivers/memory/Kconfig |   8 ++
 drivers/memory/Makefile|   1 +
 drivers/memory/mtk-smi.c   | 268 +
 include/soc/mediatek/smi.h |  58 ++
 4 files changed, 335 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 000..3714f604
--- /dev/null
+++ b/drivers/memory/mtk-smi.c
@@ -0,0 +1,268 @@
+/*
+ * Copyright (c) 2014-2015 MediaTek Inc.
+ * Author: Yong Wu 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define SMI_LARB_MMU_EN0xf00
+
+struct mtk_smi {
+   struct device   *dev;
+   struct clk  *clk_apb, *clk_smi;
+};
+
+struct mtk_smi_larb { /* larb: local arbiter */
+   struct mtk_smi  smi;
+   void __iomem*base;
+   struct device   *smi_common_dev;
+   u32 *mmu;
+};
+
+static int mtk_smi_enable(const struct mtk_smi *smi)
+{
+   int ret;
+
+   ret = pm_runtime_get_sync(smi->dev);
+   if (ret < 0)
+   return ret;
+
+   ret = clk_prepare_enable(smi->clk_apb);
+   if (ret)
+   goto err_put_pm;
+
+   ret = clk_prepare_enable(smi->clk_smi);
+   if (ret)
+   goto err_disable_apb;
+
+   return 0;
+
+err_disable_apb:
+   clk_disable_unprepare(smi->clk_apb);
+err_put_pm:
+   pm_runtime_put_sync(smi->dev);
+   return ret;
+}
+
+static void mtk_smi_disable(const struct mtk_smi *smi)
+{
+   clk_disable_unprepare(smi->clk_smi);
+   clk_disable_unprepare(smi->clk_apb);
+   pm_runtime_put_sync(smi->dev);
+}
+
+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);
+   int ret;
+
+   /* Enable the smi-common's power and clocks */
+   ret = mtk_smi_enable(common);
+   if (ret)
+   return ret;
+
+   /* Enable the larb's power and clocks */
+   ret = mtk_smi_enable(>smi);
+   if (ret) {
+   mtk_smi_disable(common);
+   return ret;
+   }
+
+   /* Configure the iommu info */
+   if (larb->mmu)
+   writel(*larb->mmu, larb->base + SMI_LARB_MMU_EN);
+
+   return 0;
+}
+
+void mtk_smi_larb_put(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_disable(>smi);
+   mtk_smi_disable(common);
+}
+
+static int
+mtk_smi_larb_bind(struct device *dev, struct device *master, void *data)
+{
+   struct mtk_smi_larb *larb = dev_get_drvdata(dev);
+   struct mtk_smi_iommu *smi_iommu = data;
+   unsigned int i;
+
+   for (i = 0; i < smi_iommu->larb_nr; i++) {
+   if (dev == smi_iommu->larb_imu[i].dev) {
+   larb->mmu = _iommu->larb_imu[i].mmu;
+   return 0;
+   }
+   }
+   return -ENODEV;
+}
+
+static void
+mtk_smi_larb_unbind(struct device *dev,