[PATCH 1/3] memory: mediatek: Add a new interface mtk_smi_larb_is_ready

[PATCH 1/3] memory: mediatek: Add a new interface mtk_smi_larb_is_ready

[Yong Wu]

Currently the iommu consumer always call iommu_present to get whether
the iommu is ready. But in MTK IOMMU, this function can't indicate
this. The IOMMU call bus_set_iommu->mtk_iommu_add_device->
mtk_iommu_attach_device to parse the iommu data, then it's able to
transfer "struct mtk_smi_iommu" to SMI-LARB, and the iommu uses the
larbs as compoents, the iommu will finish its probe until all the larbs
probe done.

If the iommu consumer(like DRM) begin to probe after the time of
calling bus_set_iommu and before the time of SMI probe finish, it
will hang like this:

[    7.832359] Call trace:
[    7.834778] [<ffffffc000764424>] mtk_smi_larb_get+0x24/0xa8
[    7.840300] [<ffffffc0005a1390>] mtk_drm_crtc_enable+0x6c/0x450

Because the larb->mmu is NULL at that time.

In order to avoid this issue, we add a new interface
(mtk_smi_larb_is_ready) for checking whether the IOMMU and SMI have
finished their probe. If it return false, the iommu consumer should
probe-defer for the IOMMU and SMI.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
Adding this interface is for avoid adjust the sequence of power
domain probe[1]. This patch-set is based on next-20160719. And
patch[1/3] base on [2], patch[3/3] base on [3], both are in the
pull tree.

[1]:http://lists.infradead.org/pipermail/linux-mediatek/2016-July/006189.html
[2]:https://patchwork.kernel.org/patch/9164067/
[3]:http://www.spinics.net/lists/arm-kernel/msg517840.html
---
 drivers/memory/mtk-smi.c   | 8 ++++++++
 include/soc/mediatek/smi.h | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
index 4afbc41..3e58cbb7 100644
--- a/drivers/memory/mtk-smi.c
+++ b/drivers/memory/mtk-smi.c
@@ -141,6 +141,14 @@ void mtk_smi_larb_put(struct device *larbdev)
 }
 EXPORT_SYMBOL_GPL(mtk_smi_larb_put);
 
+bool mtk_smi_larb_is_ready(struct device *larbdev)
+{
+	struct mtk_smi_larb *larb = dev_get_drvdata(larbdev);
+
+	return larb && larb->mmu;
+}
+EXPORT_SYMBOL_GPL(mtk_smi_larb_is_ready);
+
 static int
 mtk_smi_larb_bind(struct device *dev, struct device *master, void *data)
 {
diff --git a/include/soc/mediatek/smi.h b/include/soc/mediatek/smi.h
index 8893c5e..f0690e0 100644
--- a/include/soc/mediatek/smi.h
+++ b/include/soc/mediatek/smi.h
@@ -33,6 +33,9 @@ struct mtk_smi_iommu {
 	struct mtk_smi_larb_iommu larb_imu[MTK_LARB_NR_MAX];
 };
 
+/* Whether SMI-larb has probe done. */
+bool mtk_smi_larb_is_ready(struct device *larbdev);
+
 /*
  * mtk_smi_larb_get: Enable the power domain and clocks for this local arbiter.
  *                   It also initialize some basic setting(like iommu).
@@ -46,6 +49,11 @@ void mtk_smi_larb_put(struct device *larbdev);
 
 #else
 
+static inline bool mtk_smi_larb_is_ready(struct device *larbdev)
+{
+	return false;
+}
+
 static inline int mtk_smi_larb_get(struct device *larbdev)
 {
 	return 0;
-- 
1.8.1.1.dirty

[PATCH 2/3] drm/mediatek: Add probe-defer for MTK IOMMU and SMI

[Yong Wu]

If DRM begin to probe before SMI probe done. It will hang:

[    7.832359] Call trace:
[    7.834778] [<ffffffc000764424>] mtk_smi_larb_get+0x24/0xa8
[    7.840300] [<ffffffc0005a1390>] mtk_drm_crtc_enable+0x6c/0x450

Use the new interface(mtk_smi_larb_is_ready) to wait for IOMMU and
SMI probe done.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
index 3970fcf..3ef3124 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
@@ -21,6 +21,7 @@
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <drm/drmP.h>
+#include <soc/mediatek/smi.h>
 #include "mtk_drm_drv.h"
 #include "mtk_drm_plane.h"
 #include "mtk_drm_ddp_comp.h"
@@ -201,6 +202,10 @@ int mtk_ddp_comp_init(struct device *dev, struct device_node *node,
 	}
 	of_node_put(larb_node);
 
+	/* Wait until MTK IOMMU and SMI probe done.*/
+	if (!mtk_smi_larb_is_ready(&larb_pdev->dev))
+		return -EPROBE_DEFER;
+
 	comp->larb_dev = &larb_pdev->dev;
 
 	return 0;
-- 
1.8.1.1.dirty

[PATCH 3/3] vcodec: mediatek: Add probe-defer for MTK IOMMU and SMI

[Yong Wu]

Mediatek V4l2 depend on the IOMMU and SMI. we should add probe-defer
to wait for the IOMMU and SMI is ready.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
index 4c977b2..9e6203e 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc_pm.c
@@ -66,6 +66,11 @@ int mtk_vcodec_init_enc_pm(struct mtk_vcodec_dev *mtkdev)
 	pdev = mtkdev->plat_dev;
 	pm->dev = &pdev->dev;
 
+	/* Wait until MTK IOMMU and SMI probe done.*/
+	if (!mtk_smi_larb_is_ready(pm->larbvenc) ||
+	    !mtk_smi_larb_is_ready(pm->larbvenclt))
+		return -EPROBE_DEFER;
+
 	pm->vencpll_d2 = devm_clk_get(&pdev->dev, "venc_sel_src");
 	if (IS_ERR(pm->vencpll_d2)) {
 		mtk_v4l2_err("devm_clk_get vencpll_d2 fail");
-- 
1.8.1.1.dirty

Re: [PATCH 1/3] memory: mediatek: Add a new interface mtk_smi_larb_is_ready

[Matthias Brugger]

On 20/07/16 05:01, Yong Wu wrote:
> Currently the iommu consumer always call iommu_present to get whether
> the iommu is ready. But in MTK IOMMU, this function can't indicate
> this. The IOMMU call bus_set_iommu->mtk_iommu_add_device->
> mtk_iommu_attach_device to parse the iommu data, then it's able to
> transfer "struct mtk_smi_iommu" to SMI-LARB, and the iommu uses the
> larbs as compoents, the iommu will finish its probe until all the larbs
> probe done.
>
> If the iommu consumer(like DRM) begin to probe after the time of
> calling bus_set_iommu and before the time of SMI probe finish, it
> will hang like this:
>
> [    7.832359] Call trace:
> [    7.834778] [<ffffffc000764424>] mtk_smi_larb_get+0x24/0xa8
> [    7.840300] [<ffffffc0005a1390>] mtk_drm_crtc_enable+0x6c/0x450
>
> Because the larb->mmu is NULL at that time.
>
> In order to avoid this issue, we add a new interface
> (mtk_smi_larb_is_ready) for checking whether the IOMMU and SMI have
> finished their probe. If it return false, the iommu consumer should
> probe-defer for the IOMMU and SMI.
>

Can't we just skip the functions in the probe and call bus_set_iommu 
only if we were able to bind all components?
Something like this:

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index c3043d8..0bef49b 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -649,10 +649,14 @@ static int mtk_iommu_probe(struct platform_device 
*pdev)
         if (ret)
                 return ret;

+       ret = component_master_add_with_match(dev, &mtk_iommu_com_ops, 
match);
+       if (ret)
+               return ret;
+
         if (!iommu_present(&platform_bus_type))
                 bus_set_iommu(&platform_bus_type, &mtk_iommu_ops);

-       return component_master_add_with_match(dev, &mtk_iommu_com_ops, 
match);
+       return ret;
  } 

 

  static int mtk_iommu_remove(struct platform_device *pdev)

Regards,
Matthias

Re: [PATCH 1/3] memory: mediatek: Add a new interface mtk_smi_larb_is_ready

[Yong Wu]

On Mon, 2016-07-25 at 10:39 +0200, Matthias Brugger wrote:
> 
> On 20/07/16 05:01, Yong Wu wrote:
> > Currently the iommu consumer always call iommu_present to get whether
> > the iommu is ready. But in MTK IOMMU, this function can't indicate
> > this. The IOMMU call bus_set_iommu->mtk_iommu_add_device->
> > mtk_iommu_attach_device to parse the iommu data, then it's able to
> > transfer "struct mtk_smi_iommu" to SMI-LARB, and the iommu uses the
> > larbs as compoents, the iommu will finish its probe until all the larbs
> > probe done.
> >
> > If the iommu consumer(like DRM) begin to probe after the time of
> > calling bus_set_iommu and before the time of SMI probe finish, it
> > will hang like this:
> >
> > [    7.832359] Call trace:
> > [    7.834778] [<ffffffc000764424>] mtk_smi_larb_get+0x24/0xa8
> > [    7.840300] [<ffffffc0005a1390>] mtk_drm_crtc_enable+0x6c/0x450
> >
> > Because the larb->mmu is NULL at that time.
> >
> > In order to avoid this issue, we add a new interface
> > (mtk_smi_larb_is_ready) for checking whether the IOMMU and SMI have
> > finished their probe. If it return false, the iommu consumer should
> > probe-defer for the IOMMU and SMI.
> >
> 
> Can't we just skip the functions in the probe and call bus_set_iommu 
> only if we were able to bind all components?
> Something like this:
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index c3043d8..0bef49b 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -649,10 +649,14 @@ static int mtk_iommu_probe(struct platform_device 
> *pdev)
>          if (ret)
>                  return ret;
> 
> +       ret = component_master_add_with_match(dev, &mtk_iommu_com_ops, 
> match);
> +       if (ret)
> +               return ret;
> +
>          if (!iommu_present(&platform_bus_type))
>                  bus_set_iommu(&platform_bus_type, &mtk_iommu_ops);
> 
> -       return component_master_add_with_match(dev, &mtk_iommu_com_ops, 
> match);
> +       return ret;
>   } 

Thanks very much for your suggestion, I have tried and this seems don't
work.

I don't know much about component, so add some logs.
                                                                
 component_master_add_with_match only add the ops into a list, it won't
wait for the smi-larb probe done. Below is the log, it will return
quickly. 

[    0.095073] mtk-iommu 10205000.iommu: begin to probe 
[    0.095922] mtk-iommu 10205000.iommu: probe hw init
[    0.095948] mtk-iommu 10205000.iommu: trying to bring up master
[    0.095967] mtk-iommu 10205000.iommu: master has incomplete
components

The binder callback(mtk_iommu_bind) is called in this backtrace:

[    0.555718] [<ffff00000849e6ec>] mtk_iommu_bind+0x14/0x48
[    0.556405] [<ffff00000850a860>] try_to_bring_up_master.part.6
+0x38/0x88
[    0.557255] [<ffff00000850b014>] component_add+0x158/0x208
[    0.557954] [<ffff0000087352a4>] mtk_smi_larb_probe+0xf8/0x134
[    0.558696] [<ffff000008511838>] platform_drv_probe+0x50/0xc8
[    0.559427] [<ffff00000850fcfc>] driver_probe_device+0x224/0x2c4
[    0.560191] [<ffff00000850ff08>] __device_attach_driver+0x98/0xc8
[    0.560966] [<ffff00000850dea0>] bus_for_each_drv+0x58/0x98
[    0.561676] [<ffff00000850fa3c>] __device_attach+0xbc/0x124
[    0.562385] [<ffff0000085100a4>] device_initial_probe+0x10/0x18
[    0.563138] [<ffff00000850ef48>] bus_probe_device+0x90/0x98
[    0.563848] [<ffff00000850f3e4>] deferred_probe_work_func+0x7c/0xb0

Thus, It looks like mtk_iommu_bind is called in mtk_smi_larb_probe which
will be delayed by power-domain.
then iommu_present is true even though M4U and SMI-larb have not
finished their component binding.

> 
>  
> 
>   static int mtk_iommu_remove(struct platform_device *pdev)
> 
> Regards,
> Matthias

Re: [PATCH 1/3] memory: mediatek: Add a new interface mtk_smi_larb_is_ready

[Tomasz Figa]

Hi,

On Mon, Jul 25, 2016 at 5:39 PM, Matthias Brugger
<matthias.bgg@gmail.com> wrote:
>
>
> On 20/07/16 05:01, Yong Wu wrote:
>>
>> Currently the iommu consumer always call iommu_present to get whether
>> the iommu is ready. But in MTK IOMMU, this function can't indicate
>> this. The IOMMU call bus_set_iommu->mtk_iommu_add_device->
>> mtk_iommu_attach_device to parse the iommu data, then it's able to
>> transfer "struct mtk_smi_iommu" to SMI-LARB, and the iommu uses the
>> larbs as compoents, the iommu will finish its probe until all the larbs
>> probe done.
>>
>> If the iommu consumer(like DRM) begin to probe after the time of
>> calling bus_set_iommu and before the time of SMI probe finish, it
>> will hang like this:
>>
>> [    7.832359] Call trace:
>> [    7.834778] [<ffffffc000764424>] mtk_smi_larb_get+0x24/0xa8
>> [    7.840300] [<ffffffc0005a1390>] mtk_drm_crtc_enable+0x6c/0x450
>>
>> Because the larb->mmu is NULL at that time.
>>
>> In order to avoid this issue, we add a new interface
>> (mtk_smi_larb_is_ready) for checking whether the IOMMU and SMI have
>> finished their probe. If it return false, the iommu consumer should
>> probe-defer for the IOMMU and SMI.
>>
>
> Can't we just skip the functions in the probe and call bus_set_iommu only if
> we were able to bind all components?
> Something like this:

Note that we have to call bus_set_iommu() and actually have
.add_device() and .attach_device() called before any of the slave
devices probe. I found a similar problem with rockchip IOMMU after
adding power domain and runtime PM handling there. I also found that
current design of IOMMU core and related DMA mapping code is utterly
broken regarding the device add/probe ordering (no support for
deferring things properly).

So my idea is to keep .add_device() as is, since typically it doesn't
seem to require anything from the IOMMU hardware and just initializes
some per-device data, but make .attach_device() being able to defer
probe of that device if respective IOMMU has not probed yet. I'm still
in process of figuring out the right way to achieve it, though...

Best regards,
Tomasz