Hi Matthias,
Thanks very much for your review.
On Mon, 2018-12-03 at 00:56 +0100, Matthias Brugger wrote:
>
> On 17/11/2018 03:35, Yong Wu wrote:
> > The M4U IP blocks in mt8183 is MediaTek's generation2 M4U which use
> > the ARM Short-descriptor like mt8173, and most of the HW registers
> > are the same.
> >
> > Here list main changes in mt8183:
> > 1) mt8183 has only one M4U HW like mt8173.
>
> That's a change?
I guess I should use "difference" rather than "changes".
(mt8173 and mt8183 have only one M4u HW while mt2712 have two.)
>
> > 2) mt8183 don't have its "bclk" clock, the M4U use the EMI clock
> > which has already been enabled before kernel.
> > 3) mt8183 can support the dram over 4GB, but it don't call this "4GB
> > mode".
> > 4) mt8183 pgtable base register(0x0) extend bit[1:0] which represent
> > the bit[33:32] in the physical address of the pgtable base, But the
> > standard ttbr0[1] means the S bit which is enabled defaultly, Hence,
> > we add a mask.
> > 5) mt8183 HW has a GALS modules, the SMI should add "gals" clock
> > support.
> > 6) the larb-id in smi-common has been remapped. This means the
> > larb-id reported in the mtk_iommu_isr is not the real larb-id, here
> > is the remapping relationship of mt8183:
>
> This whole list is an indicator that you will need several enhancements to the
> existing code before you can add mt8183 support.
> Please do so in independent patches (e.g. gals modules, remap logic)
OK. I will separate them into 2 new preparing patches.
> Regarding making bclk optional, I think it would be better to set it to NULL for
> mt8183 otherwise the driver will work with a broken device tree on systems that
> actually need the bclk. Same holds for gals0 and gals1.
OK. From your comment below, I will add a "has_bclk" in the platform
data and change code like this:
if (data->plat_data->has_bclk) {
data->bclk = devm_clk_get();
if (IS_ERR(data->bclk))
return PTR_ERR(data->bclk).
} else {
data->bclk = NULL;
}
>
> > M4U
> > |
> > ---------------------------------------------
> > | SMI common |
> > -0-----7-----5-----6-----1-----2------3-----4- <- Id remapped
> > | | | | | | | |
> > larb0 larb1 IPU0 IPU1 larb4 larb5 larb6 CCU
> > disp vdec img cam venc img cam
> > As above, larb0 connects with the id 0 in smi-common.
> > larb1 connects with the id 7 in smi-common.
> > ...
> > Take a example, if the larb-id reported in the mtk_iommu_isr is 7,
> > actually it is larb1(vdec).
> >
> > Signed-off-by: Yong Wu <yong.wu@xxxxxxxxxxxx>
> > ---
> > drivers/iommu/mtk_iommu.c | 38 ++++++++++++++++++++++++++++----------
> > drivers/iommu/mtk_iommu.h | 5 +++++
> > drivers/memory/mtk-smi.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 80 insertions(+), 10 deletions(-)
> >
>
> [...]
>
> > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> > index a243047..e5fd8ee 100644
> > --- a/drivers/iommu/mtk_iommu.h
> > +++ b/drivers/iommu/mtk_iommu.h
> > @@ -39,11 +39,16 @@ enum mtk_iommu_plat {
> > M4U_MT2701,
> > M4U_MT2712,
> > M4U_MT8173,
> > + M4U_MT8183,
> > };
> >
> > struct mtk_iommu_plat_data {
> > enum mtk_iommu_plat m4u_plat;
> > bool has_4gb_mode;
> > +
> > + /* The larb-id may be remapped in the smi-common. */
> > + bool larbid_remap_enable;
> > + unsigned int larbid_remapped[MTK_LARB_NR_MAX];
>
> Please add new features to the driver as single patches. Don't add this kind of
> things in the patch where you enable a new SoC.
OK.
>
> > };
> >
> > struct mtk_iommu_domain;
> > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> > index e37e54b..979153b 100644
> > --- a/drivers/memory/mtk-smi.c
> > +++ b/drivers/memory/mtk-smi.c
> > @@ -59,6 +59,7 @@ struct mtk_smi_larb_gen {
> > struct mtk_smi {
> > struct device *dev;
> > struct clk *clk_apb, *clk_smi;
> > + struct clk *clk_gals0, *clk_gals1;
> > struct clk *clk_async; /*only needed by mt2701*/
> > void __iomem *smi_ao_base;
> > };
> > @@ -93,8 +94,20 @@ static int mtk_smi_enable(const struct mtk_smi *smi)
> > if (ret)
> > goto err_disable_apb;
> >
> > + ret = clk_prepare_enable(smi->clk_gals0);
> > + if (ret)
> > + goto err_disable_smi;
> > +
> > + ret = clk_prepare_enable(smi->clk_gals1);
> > + if (ret)
> > + goto err_disable_gals0;
> > +> return 0;
> >
> > +err_disable_gals0:
> > + clk_disable_unprepare(smi->clk_gals0);
> > +err_disable_smi:
> > + clk_disable_unprepare(smi->clk_smi);
> > err_disable_apb:
> > clk_disable_unprepare(smi->clk_apb);
> > err_put_pm:
> > @@ -104,6 +117,8 @@ static int mtk_smi_enable(const struct mtk_smi *smi)
> >
> > static void mtk_smi_disable(const struct mtk_smi *smi)
> > {
> > + clk_disable_unprepare(smi->clk_gals1);
> > + clk_disable_unprepare(smi->clk_gals0);
> > clk_disable_unprepare(smi->clk_smi);
> > clk_disable_unprepare(smi->clk_apb);
> > pm_runtime_put_sync(smi->dev);
> > @@ -262,6 +277,12 @@ static void mtk_smi_larb_config_port_gen1(struct device *dev)
> > .larb_special_mask = BIT(8) | BIT(9), /* bdpsys */
> > };
> >
> > +static const struct mtk_smi_larb_gen mtk_smi_larb_mt8183 = {
> > + .need_larbid = true,
> > + .config_port = mtk_smi_larb_config_port_gen2_general,
> > + .larb_special_mask = BIT(2) | BIT(3) | BIT(7), /* IPU0 | IPU1 | CCU */
> > +};
> > +
> > static const struct of_device_id mtk_smi_larb_of_ids[] = {
> > {
> > .compatible = "mediatek,mt8173-smi-larb",
> > @@ -275,6 +296,10 @@ static void mtk_smi_larb_config_port_gen1(struct device *dev)
> > .compatible = "mediatek,mt2712-smi-larb",
> > .data = &mtk_smi_larb_mt2712
> > },
> > + {
> > + .compatible = "mediatek,mt8183-smi-larb",
> > + .data = &mtk_smi_larb_mt8183
> > + },
> > {}
> > };
> >
> > @@ -304,6 +329,12 @@ static int mtk_smi_larb_probe(struct platform_device *pdev)
> > larb->smi.clk_smi = devm_clk_get(dev, "smi");
> > if (IS_ERR(larb->smi.clk_smi))
> > return PTR_ERR(larb->smi.clk_smi);
> > +
> > + larb->smi.clk_gals0 = devm_clk_get(dev, "gals");> + if (PTR_ERR(larb->smi.clk_gals0) == -ENOENT)
> > + larb->smi.clk_gals0 = NULL;
> > + else if (IS_ERR(larb->smi.clk_gals0))
> > + return PTR_ERR(larb->smi.clk_gals0);
> > larb->smi.dev = dev;
> >
> > if (larb->larb_gen->need_larbid) {
> > @@ -364,6 +395,10 @@ static int mtk_smi_larb_remove(struct platform_device *pdev)
> > .compatible = "mediatek,mt2712-smi-common",
> > .data = (void *)MTK_SMI_GEN2
> > },
> > + {
> > + .compatible = "mediatek,mt8183-smi-common",
> > + .data = (void *)MTK_SMI_GEN2
> > + },
> > {}
> > };
> >
> > @@ -388,6 +423,18 @@ static int mtk_smi_common_probe(struct platform_device *pdev)
> > if (IS_ERR(common->clk_smi))
> > return PTR_ERR(common->clk_smi);
> >
> > + common->clk_gals0 = devm_clk_get(dev, "gals0");
> > + if (PTR_ERR(common->clk_gals0) == -ENOENT)
> > + common->clk_gals0 = NULL;
> > + else if (IS_ERR(common->clk_gals0))
> > + return PTR_ERR(common->clk_gals0);
> > +
> > + common->clk_gals1 = devm_clk_get(dev, "gals1");
> > + if (PTR_ERR(common->clk_gals1) == -ENOENT)
> > + common->clk_gals1 = NULL;
> > + else if (IS_ERR(common->clk_gals1))
> > + return PTR_ERR(common->clk_gals1);
> > +
>
> I'd prefer to check for clk_gals[01] for SoCs that need this clocks instead of
> making them optional.
OK. like "bclk", I will add a "has_gals" in the SOCs data.
>
> Regards,
> Matthias
