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

2015-12-15 Thread Yong Wu 
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  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 
> >> > ---
> >> >   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(>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)
> >> > +{
> >> > +

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

2015-12-15 Thread Yong Wu 
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  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 
> >> > ---
> >> >   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(>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

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

2015-12-14 Thread Daniel Kurtz 
Hi Yong,

On Tue, Dec 15, 2015 at 10:38 AM, Yong Wu  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 
>> > ---
>> >   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(>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

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

2015-12-14 Thread Yong Wu 
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 
> > ---
> >   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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-12-14 Thread Matthias Brugger 
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 
> ---
>   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 000..3562081
> --- /dev/null
> +++ b/drivers/memory/mtk-smi.c
> @@ -0,0 +1,297 @@
> +/*
> + * 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_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.

>

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

2015-12-14 Thread Yong Wu 
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 
> > ---
> >   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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-12-14 Thread Daniel Kurtz 
Hi Yong,

On Tue, Dec 15, 2015 at 10:38 AM, Yong Wu  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 
>> > ---
>> >   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(>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

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

2015-12-14 Thread Matthias Brugger 
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 
> ---
>   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 000..3562081
> --- /dev/null
> +++ b/drivers/memory/mtk-smi.c
> @@ -0,0 +1,297 @@
> +/*
> + * 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_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

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

2015-12-10 Thread Yong Wu 
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 
> ---

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-12-10 Thread Yong Wu 
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 
> ---

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-12-08 Thread 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 
---
  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 000..3562081
--- /dev/null
+++ b/drivers/memory/mtk-smi.c
@@ -0,0 +1,297 @@
+/*
+ * 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
+#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;
+
+   /*

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

2015-12-08 Thread 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 
---
  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 000..3562081
--- /dev/null
+++ b/drivers/memory/mtk-smi.c
@@ -0,0 +1,297 @@
+/*
+ * 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
+#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)
+