Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Will Deacon]

On Mon, Mar 09, 2015 at 12:11:43PM +0000, Yong Wu wrote:
> On Fri, 2015-03-06 at 10:58 +0000, Will Deacon wrote:
> > On Fri, Mar 06, 2015 at 10:48:17AM +0000, yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> > > From: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > 
> > > This patch adds support for mediatek m4u (MultiMedia Memory Management Unit).
> > > Currently this only supports m4u gen 2 with 2 levels of page table on mt8173.
> > 
> > [...]
> > 
> > > +/* 2 level pagetable: pgd -> pte */
> > > +#define F_PTE_TYPE_GET(regval)  (regval & 0x3)
> > > +#define F_PTE_TYPE_LARGE         BIT(0)
> > > +#define F_PTE_TYPE_SMALL         BIT(1)
> > > +#define F_PTE_B_BIT              BIT(2)
> > > +#define F_PTE_C_BIT              BIT(3)
> > > +#define F_PTE_BIT32_BIT          BIT(9)
> > > +#define F_PTE_S_BIT              BIT(10)
> > > +#define F_PTE_NG_BIT             BIT(11)
> > > +#define F_PTE_PA_LARGE_MSK            (~0UL << 16)
> > > +#define F_PTE_PA_LARGE_GET(regval)    ((regval >> 16) & 0xffff)
> > > +#define F_PTE_PA_SMALL_MSK            (~0UL << 12)
> > > +#define F_PTE_PA_SMALL_GET(regval)    ((regval >> 12) & (~0))
> > > +#define F_PTE_TYPE_IS_LARGE_PAGE(pte) ((imu_pte_val(pte) & 0x3) == \
> > > +                                       F_PTE_TYPE_LARGE)
> > > +#define F_PTE_TYPE_IS_SMALL_PAGE(pte) ((imu_pte_val(pte) & 0x3) == \
> > > +                                       F_PTE_TYPE_SMALL)
> > 
> > This looks like the ARM short-descriptor format to me. Could you please
> > add a new page table format to the io-pgtable code, so that other IOMMU
> > drivers can make use of this? I know there was some interest in using
> > short descriptor for the ARM SMMU, for example.
>     Currently I not familiar with the io-pgtable,I may need some time
> for it and the ARM short-descriptor. 

Well, you can read the LPAE version I wrote in io-pgtable-arm.c for some
inspiration (it's used by arm-smmu.c and ipmmu-vmsa.c).

>     And there are some difference between mediatek's pagetable with the
> standard short-descriptor, like bit 9. we use it for the dram over 4GB.
> Then how should we do if there are some difference. 

That can easily be handled using a quirk (see, for example,
IO_PGTABLE_QUIRK_ARM_NS).

Will
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Yong Wu]

Hi Tomasz,
   Thanks very much for your review. please help check below.
The others I will fix in the next version.

Hi Robin,
   There are some place I would like you can have a look and give me
some suggestion.

On Wed, 2015-03-11 at 19:53 +0900, Tomasz Figa wrote:
> Hi,
> 
> Please find next part of my comments inline.
> 
> On Fri, Mar 6, 2015 at 7:48 PM,  <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> 
> [snip]
> 
> > +/*
> > + * pimudev is a global var for dma_alloc_coherent.
> > + * It is not accepatable, we will delete it if "domain_alloc" is enabled
> 
> It looks like we indeed need to use dma_alloc_coherent() and we don't
> have a good way to pass the device pointer to domain_init callback.
> 
> If you don't expect SoCs in the nearest future to have multiple M4U
> blocks, then I guess this global variable could stay, after changing
> the comment into an explanation why it's correct. Also it should be
> moved to the top of the file, below #include directives, as this is
> where usually global variables are located.
@Robin,
     We have merged this patch[0] in order to delete the global var, But
it seems that your patch of "arm64:IOMMU" isn't based on it right row.
it will build fail.

[0]:http://lists.linuxfoundation.org/pipermail/iommu/2015-January/011939.html

> > + */
> > +static struct device *pimudev;
> > +
[snip]
> > +
> > +static int mtk_iommu_attach_device(struct iommu_domain *domain,
> > +                                  struct device *dev)
> > +{
> > +       unsigned long flags;
> > +       struct mtk_iommu_domain *priv = domain->priv;
> > +       struct mtk_iommu_info *piommu = priv->piommuinfo;
> > +       struct of_phandle_args out_args = {0};
> > +       struct device *imudev;
> > +       unsigned int i = 0;
> > +
> > +       if (!piommu)
> 
> Could you explain when this can happen?
	If we call arch_setup_dma_ops to create a iommu domain,
it will enter iommu_dma_attach_device, then enter here. At that time, we
don't add the private data to this "struct iommu_domain *".
@Robin, Could this be improved?
> 
> > +               goto imudev;
> 
> return 0;
> 
> > +       else
> 
> No else needed.
> 
> > +               imudev = piommu->dev;
> > +
> > +       spin_lock_irqsave(&priv->portlock, flags);
> 
> What is protected by this spinlock?
	We will write a register of the local arbiter while config port. If
some modules are in the same local arbiter, it may be overwrite. so I
add it here.
> 
> > +
> > +       while (!of_parse_phandle_with_args(dev->of_node, "iommus",
> > +                                          "#iommu-cells", i, &out_args)) {
> > +               if (1 == out_args.args_count) {
> 
> Can we be sure that this is actually referring to our IOMMU?
> 
> Maybe this should be rewritten to
> 
> if (out_args.np != imudev->of_node)
>         continue;
> if (out_args.args_count != 1) {
>         dev_err(imudev, "invalid #iommu-cells property for IOMMU %s\n",
> 
> }
> 
> > +                       unsigned int portid = out_args.args[0];
> > +
> > +                       dev_dbg(dev, "iommu add port:%d\n", portid);
> 
> imudev should be used here instead of dev.
> 
> > +
> > +                       mtk_iommu_config_port(piommu, portid);
> > +
> > +                       if (i == 0)
> > +                               dev->archdata.dma_ops =
> > +                                       piommu->dev->archdata.dma_ops;
> 
> Shouldn't this be set automatically by IOMMU or DMA mapping core?
@Robin, 
     In the original "arm_iommu_attach_device" of arm/mm, it will call
set_dma_ops to add iommu_ops for each iommu device.
But iommu_dma_attach_device don't help this, so I have to add it here.
Could this be improved?
> 
> > +               }
> > +               i++;
> > +       }
> > +
> > +       spin_unlock_irqrestore(&priv->portlock, flags);
> > +
> > +imudev:
> > +       return 0;
> > +}
> > +
> > +static void mtk_iommu_detach_device(struct iommu_domain *domain,
> > +                                   struct device *dev)
> > +{
> 
> No hardware (de)configuration or clean-up necessary?
I will add it. Actually we design like this:If a device have attached to
iommu domain, it won't detach from it.
> 
> > +}
> > +
[snip]
> 
> > +
> > +       piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,
> 
> style: Operators like * should have space on both sides.
> 
> > +                                         GFP_KERNEL);
> 
> Shouldn't dma_alloc_coherent() be used for this?
     We don't care the data in it. I think they are the same. Could you
help tell me why dma_alloc_coherent may be better.
> 
> > +       if (!piommu->protect_va)
> > +               goto protect_err;
> 
> Please return -ENOMEM here directly, as there is nothing to clean up
> in this case.
> 
[snip]
> 
> > +               dev_err(piommu->dev, "IRQ request %d failed\n",
> > +                       piommu->irq);
> > +               goto hw_err;
> > +       }
> > +
> > +       iommu_set_fault_handler(domain, mtk_iommu_fault_handler, piommu);
> 
> I don't see any other drivers doing this. Isn't this for upper layers,
> so that they can set their own generic fault handlers?
     I think that this function is related with the iommu domain, we
have only one multimedia iommu domain. so I add it after the iommu
domain are created.
> 
> > +
> > +       dev_set_drvdata(piommu->dev, piommu);
> 
> This should be set before allowing the interrupt to fire. In other
> words, the driver should be fully set up at the time of enabling the
> IRQ.
> 
> > +
> > +       return 0;
> 
> style: Missing blank line.
> 
> > +hw_err:
> > +       arch_teardown_dma_ops(piommu->dev);
> > +pte_err:
> > +       kmem_cache_destroy(piommu->m4u_pte_kmem);
> > +protect_err:
> > +       dev_err(piommu->dev, "probe error\n");
> 
> Please replace this with specific messages for all errors (in case the
> called function doesn't already print one like kmalloc and friends).
> 
> > +       return 0;
> 
> Returning 0, which means success, doesn't look like a good idea for
> signalling a failure. Please return the correct error code as received
> from function that errors out if possible.
> 
> End of part 3.
> 
> Best regards,
> Tomasz

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Robin Murphy]

On 18/03/15 11:22, Yong Wu wrote:
> Hi Tomasz,
>     Thanks very much for your review. please help check below.
> The others I will fix in the next version.
>
> Hi Robin,
>     There are some place I would like you can have a look and give me
> some suggestion.
>
> On Wed, 2015-03-11 at 19:53 +0900, Tomasz Figa wrote:
>> Hi,
>>
>> Please find next part of my comments inline.
>>
>> On Fri, Mar 6, 2015 at 7:48 PM,  <yong.wu@mediatek.com> wrote:
>>
>> [snip]
>>
>>> +/*
>>> + * pimudev is a global var for dma_alloc_coherent.
>>> + * It is not accepatable, we will delete it if "domain_alloc" is enabled
>>
>> It looks like we indeed need to use dma_alloc_coherent() and we don't
>> have a good way to pass the device pointer to domain_init callback.
>>
>> If you don't expect SoCs in the nearest future to have multiple M4U
>> blocks, then I guess this global variable could stay, after changing
>> the comment into an explanation why it's correct. Also it should be
>> moved to the top of the file, below #include directives, as this is
>> where usually global variables are located.
> @Robin,
>       We have merged this patch[0] in order to delete the global var, But
> it seems that your patch of "arm64:IOMMU" isn't based on it right row.
> it will build fail.

Yeah, I've not yet managed to try pulling in that series (much as I 
approve of it), partly as I know doing so is going to lean towards a 
not-insignificant rework and I'd rather avoid picking up more unmerged 
dependencies to block getting _something_ in for arm64 (which we can 
then improve).

>
> [0]:http://lists.linuxfoundation.org/pipermail/iommu/2015-January/011939.html
>
>>> + */
>>> +static struct device *pimudev;
>>> +
> [snip]
>>> +
>>> +static int mtk_iommu_attach_device(struct iommu_domain *domain,
>>> +                                  struct device *dev)
>>> +{
>>> +       unsigned long flags;
>>> +       struct mtk_iommu_domain *priv = domain->priv;
>>> +       struct mtk_iommu_info *piommu = priv->piommuinfo;
>>> +       struct of_phandle_args out_args = {0};
>>> +       struct device *imudev;
>>> +       unsigned int i = 0;
>>> +
>>> +       if (!piommu)
>>
>> Could you explain when this can happen?
> 	If we call arch_setup_dma_ops to create a iommu domain,
> it will enter iommu_dma_attach_device, then enter here. At that time, we
> don't add the private data to this "struct iommu_domain *".
> @Robin, Could this be improved?

Calling arch_setup_dma_ops() from the driver looks plain wrong, 
especially given that you apparently attach the IOMMU to itself - if you 
want your own domain you should use iommu_dma_create_domain(). I admit 
that still leaves you having to dance around a bit in order to tear down 
the automatic domains for now, but hopefully we'll get the core code 
sorted out sooner rather than later.

>>
>>> +               goto imudev;
>>
>> return 0;
>>
>>> +       else
>>
>> No else needed.
>>
>>> +               imudev = piommu->dev;
>>> +
>>> +       spin_lock_irqsave(&priv->portlock, flags);
>>
>> What is protected by this spinlock?
> 	We will write a register of the local arbiter while config port. If
> some modules are in the same local arbiter, it may be overwrite. so I
> add it here.
>>
>>> +
>>> +       while (!of_parse_phandle_with_args(dev->of_node, "iommus",
>>> +                                          "#iommu-cells", i, &out_args)) {
>>> +               if (1 == out_args.args_count) {
>>
>> Can we be sure that this is actually referring to our IOMMU?
>>
>> Maybe this should be rewritten to
>>
>> if (out_args.np != imudev->of_node)
>>          continue;
>> if (out_args.args_count != 1) {
>>          dev_err(imudev, "invalid #iommu-cells property for IOMMU %s\n",
>>
>> }
>>
>>> +                       unsigned int portid = out_args.args[0];
>>> +
>>> +                       dev_dbg(dev, "iommu add port:%d\n", portid);
>>
>> imudev should be used here instead of dev.
>>
>>> +
>>> +                       mtk_iommu_config_port(piommu, portid);
>>> +
>>> +                       if (i == 0)
>>> +                               dev->archdata.dma_ops =
>>> +                                       piommu->dev->archdata.dma_ops;
>>
>> Shouldn't this be set automatically by IOMMU or DMA mapping core?
> @Robin,
>       In the original "arm_iommu_attach_device" of arm/mm, it will call
> set_dma_ops to add iommu_ops for each iommu device.
> But iommu_dma_attach_device don't help this, so I have to add it here.
> Could this be improved?

If you implemented a simple of_xlate callback so that the core code 
handles the dma_ops as intended, I think the simplest cheat would be to 
check the client device's domain, either on attachment or when they 
start mapping/unmapping, and move them to your own domain if necessary. 
I'm putting together a v3 of the DMA mapping series, so I'll have a look 
to see if I can squeeze in a way to make that a bit less painful until 
we solve it properly.


Robin.

>>
>>> +               }
>>> +               i++;
>>> +       }
>>> +
>>> +       spin_unlock_irqrestore(&priv->portlock, flags);
>>> +
>>> +imudev:
>>> +       return 0;
>>> +}
>>> +
>>> +static void mtk_iommu_detach_device(struct iommu_domain *domain,
>>> +                                   struct device *dev)
>>> +{
>>
>> No hardware (de)configuration or clean-up necessary?
> I will add it. Actually we design like this:If a device have attached to
> iommu domain, it won't detach from it.
>>
>>> +}
>>> +
> [snip]
>>
>>> +
>>> +       piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,
>>
>> style: Operators like * should have space on both sides.
>>
>>> +                                         GFP_KERNEL);
>>
>> Shouldn't dma_alloc_coherent() be used for this?
>       We don't care the data in it. I think they are the same. Could you
> help tell me why dma_alloc_coherent may be better.
>>
>>> +       if (!piommu->protect_va)
>>> +               goto protect_err;
>>
>> Please return -ENOMEM here directly, as there is nothing to clean up
>> in this case.
>>
> [snip]
>>
>>> +               dev_err(piommu->dev, "IRQ request %d failed\n",
>>> +                       piommu->irq);
>>> +               goto hw_err;
>>> +       }
>>> +
>>> +       iommu_set_fault_handler(domain, mtk_iommu_fault_handler, piommu);
>>
>> I don't see any other drivers doing this. Isn't this for upper layers,
>> so that they can set their own generic fault handlers?
>       I think that this function is related with the iommu domain, we
> have only one multimedia iommu domain. so I add it after the iommu
> domain are created.
>>
>>> +
>>> +       dev_set_drvdata(piommu->dev, piommu);
>>
>> This should be set before allowing the interrupt to fire. In other
>> words, the driver should be fully set up at the time of enabling the
>> IRQ.
>>
>>> +
>>> +       return 0;
>>
>> style: Missing blank line.
>>
>>> +hw_err:
>>> +       arch_teardown_dma_ops(piommu->dev);
>>> +pte_err:
>>> +       kmem_cache_destroy(piommu->m4u_pte_kmem);
>>> +protect_err:
>>> +       dev_err(piommu->dev, "probe error\n");
>>
>> Please replace this with specific messages for all errors (in case the
>> called function doesn't already print one like kmalloc and friends).
>>
>>> +       return 0;
>>
>> Returning 0, which means success, doesn't look like a good idea for
>> signalling a failure. Please return the correct error code as received
>> from function that errors out if possible.
>>
>> End of part 3.
>>
>> Best regards,
>> Tomasz
>
>
>

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Yong Wu]

Hi Robin,
      Thanks very much for your confirm.
      About the v3 of the DMA-mapping, I have some question below.

On Fri, 2015-03-20 at 19:14 +0000, Robin Murphy wrote:
> On 18/03/15 11:22, Yong Wu wrote:
> > Hi Tomasz,
> >     Thanks very much for your review. please help check below.
> > The others I will fix in the next version.
> >
> > Hi Robin,
> >     There are some place I would like you can have a look and give me
> > some suggestion.
> >
> > On Wed, 2015-03-11 at 19:53 +0900, Tomasz Figa wrote:
> >> Hi,
> >>
> >> Please find next part of my comments inline.
> >>
> >>> +/*
> >>> + * pimudev is a global var for dma_alloc_coherent.
> >>> + * It is not accepatable, we will delete it if "domain_alloc" is enabled
> >>
> >> It looks like we indeed need to use dma_alloc_coherent() and we don't
> >> have a good way to pass the device pointer to domain_init callback.
> >>
> >> If you don't expect SoCs in the nearest future to have multiple M4U
> >> blocks, then I guess this global variable could stay, after changing
> >> the comment into an explanation why it's correct. Also it should be
> >> moved to the top of the file, below #include directives, as this is
> >> where usually global variables are located.
> > @Robin,
> >       We have merged this patch[0] in order to delete the global var, But
> > it seems that your patch of "arm64:IOMMU" isn't based on it right row.
> > it will build fail.
> 
> Yeah, I've not yet managed to try pulling in that series (much as I 
> approve of it), partly as I know doing so is going to lean towards a 
> not-insignificant rework and I'd rather avoid picking up more unmerged 
> dependencies to block getting _something_ in for arm64 (which we can 
> then improve).
> 
> >
> > [0]:http://lists.linuxfoundation.org/pipermail/iommu/2015-January/011939.html
> >
[snip]
> 
> Calling arch_setup_dma_ops() from the driver looks plain wrong, 
> especially given that you apparently attach the IOMMU to itself - if you 
> want your own domain you should use iommu_dma_create_domain(). I admit 
> that still leaves you having to dance around a bit in order to tear down 
> the automatic domains for now, but hopefully we'll get the core code 
> sorted out sooner rather than later.
> >>> +
> >>> +                       mtk_iommu_config_port(piommu, portid);
> >>> +
> >>> +                       if (i == 0)
> >>> +                               dev->archdata.dma_ops =
> >>> +                                       piommu->dev->archdata.dma_ops;
> >>
> >> Shouldn't this be set automatically by IOMMU or DMA mapping core?
> > @Robin,
> >       In the original "arm_iommu_attach_device" of arm/mm, it will call
> > set_dma_ops to add iommu_ops for each iommu device.
> > But iommu_dma_attach_device don't help this, so I have to add it here.
> > Could this be improved?
> 
> If you implemented a simple of_xlate callback so that the core code 
> handles the dma_ops as intended, I think the simplest cheat would be to 
> check the client device's domain, either on attachment or when they 
> start mapping/unmapping, and move them to your own domain if necessary. 
> I'm putting together a v3 of the DMA mapping series, so I'll have a look 
> to see if I can squeeze in a way to make that a bit less painful until 
> we solve it properly.
> 
> 
> Robin.
> 
      I have implemented a simple of_xlate, but I can’t get the standard
struct dma_map_ops “iommu_dma_ops” to assigned it to the client device.
So the v3 of dma mapping will improve this issue?  

      And Is the v3 of the DMA-mapping based on 4.0-rc1? because we
expect it could contain will’s io-pagetable.

      And when the v3 will be ready?
> >>
> >>> +               }
> >>> +               i++;
> >>> +       }
> >>> +
> >>> +       spin_unlock_irqrestore(&priv->portlock, flags);
> >>> +
> >>> +imudev:
> >>> +       return 0;
> >>> +}
> >>> +
> >>> +static void mtk_iommu_detach_device(struct iommu_domain *domain,
> >>> +                                   struct device *dev)
> >>> +{
> >>
> >> No hardware (de)configuration or clean-up necessary?
> > I will add it. Actually we design like this:If a device have attached to
> > iommu domain, it won't detach from it.
> >>
> >>> +}
> >>> +
> > [snip]


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

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Tomasz Figa]

Hi Yong Wu,

Sorry for long delay, I had to figure out some time to look at this again.

On Wed, Mar 18, 2015 at 8:22 PM, Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>>
>> > +               imudev = piommu->dev;
>> > +
>> > +       spin_lock_irqsave(&priv->portlock, flags);
>>
>> What is protected by this spinlock?
>         We will write a register of the local arbiter while config port. If
> some modules are in the same local arbiter, it may be overwrite. so I
> add it here.
>>

OK. Maybe it could be called larb_lock then? It would be good to have
structures or code that should be running under this spinlock
annotated with proper comments. And purpose of the lock documented in
a comment as well (probably in a kerneldoc-style documentation of
priv).

>> > +static void mtk_iommu_detach_device(struct iommu_domain *domain,
>> > +                                   struct device *dev)
>> > +{
>>
>> No hardware (de)configuration or clean-up necessary?
> I will add it. Actually we design like this:If a device have attached to
> iommu domain, it won't detach from it.

Isn't proper clean-up required for module removal? Some drivers might
be required to be loadable modules, which should be unloadable.

>>
>> > +
>> > +       piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,
>>
>> style: Operators like * should have space on both sides.
>>
>> > +                                         GFP_KERNEL);
>>
>> Shouldn't dma_alloc_coherent() be used for this?
>      We don't care the data in it. I think they are the same. Could you
> help tell me why dma_alloc_coherent may be better.

Can you guarantee that at the time you allocate the memory using
devm_kmalloc() the memory is not dirty (i.e. some write back data are
stored in CPU cache) and is not going to be written back in some time,
overwriting data put there by IOMMU hardware?

>> > +
>> > +       iommu_set_fault_handler(domain, mtk_iommu_fault_handler, piommu);
>>
>> I don't see any other drivers doing this. Isn't this for upper layers,
>> so that they can set their own generic fault handlers?
>      I think that this function is related with the iommu domain, we
> have only one multimedia iommu domain. so I add it after the iommu
> domain are created.

No, this function is for drivers of IOMMU clients (i.e. master IP
blocks) which want to subscribe to page fault to do things like paging
on demand and so on. It shouldn't be called by IOMMU driver. Please
see other IOMMU drivers, for example rockchip-iommmu.c.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Yong Wu]

Hi Tomasz,

     Thanks very much for you suggestion and explain so detail.
     please help check below.

On Fri, 2015-03-27 at 18:41 +0900, Tomasz Figa wrote:
> Hi Yong Wu,
> 
> Sorry for long delay, I had to figure out some time to look at this again.
> 
> On Wed, Mar 18, 2015 at 8:22 PM, Yong Wu <yong.wu@mediatek.com> wrote:
> >>
> >> > +               imudev = piommu->dev;
> >> > +
> >> > +       spin_lock_irqsave(&priv->portlock, flags);
> >>
> >> What is protected by this spinlock?
> >         We will write a register of the local arbiter while config port. If
> > some modules are in the same local arbiter, it may be overwrite. so I
> > add it here.
> >>
> 
> OK. Maybe it could be called larb_lock then? It would be good to have
> structures or code that should be running under this spinlock
> annotated with proper comments. And purpose of the lock documented in
> a comment as well (probably in a kerneldoc-style documentation of
> priv).
   Thanks. I have move the spinlock into the smi driver, it will lock
for writing the local arbiter regsiter only.
> 
> >> > +static void mtk_iommu_detach_device(struct iommu_domain *domain,
> >> > +                                   struct device *dev)
> >> > +{
> >>
> >> No hardware (de)configuration or clean-up necessary?
> > I will add it. Actually we design like this:If a device have attached to
> > iommu domain, it won't detach from it.
> 
> Isn't proper clean-up required for module removal? Some drivers might
> be required to be loadable modules, which should be unloadable.
> 
> >>
> >> > +
> >> > +       piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,
> >>
> >> style: Operators like * should have space on both sides.
> >>
> >> > +                                         GFP_KERNEL);
> >>
> >> Shouldn't dma_alloc_coherent() be used for this?
> >      We don't care the data in it. I think they are the same. Could you
> > help tell me why dma_alloc_coherent may be better.
> 
> Can you guarantee that at the time you allocate the memory using
> devm_kmalloc() the memory is not dirty (i.e. some write back data are
> stored in CPU cache) and is not going to be written back in some time,
> overwriting data put there by IOMMU hardware?
> 
As I noted in the function "mtk_iommu_hw_init":

       /* protect memory,HW will write here while translation fault */
       protectpa = __virt_to_phys(piommu->protect_va);

     We don’t care the content of this buffer, It is ok even though its
data is dirty.
    It seem to be a the protect memory. While a translation fault
happened, The iommu HW will overwrite here instead of writing to the
fault physical address which may be 0 or some random address.

> >> > +
> >> > +       iommu_set_fault_handler(domain, mtk_iommu_fault_handler, piommu);
> >>
> >> I don't see any other drivers doing this. Isn't this for upper layers,
> >> so that they can set their own generic fault handlers?
> >      I think that this function is related with the iommu domain, we
> > have only one multimedia iommu domain. so I add it after the iommu
> > domain are created.
> 
> No, this function is for drivers of IOMMU clients (i.e. master IP
> blocks) which want to subscribe to page fault to do things like paging
> on demand and so on. It shouldn't be called by IOMMU driver. Please
> see other IOMMU drivers, for example rockchip-iommmu.c.
     Thanks. I have read it. I will delete it and print the error info
in the ISR. Also call the report_iommu_fault in the ISR.

> Best regards,
> Tomasz


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

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Tomasz Figa]

On Tue, Apr 14, 2015 at 3:31 PM, Yong Wu <yong.wu@mediatek.com> wrote:
>> >>
>> >> > +
>> >> > +       piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,
>> >>
>> >> style: Operators like * should have space on both sides.
>> >>
>> >> > +                                         GFP_KERNEL);
>> >>
>> >> Shouldn't dma_alloc_coherent() be used for this?
>> >      We don't care the data in it. I think they are the same. Could you
>> > help tell me why dma_alloc_coherent may be better.
>>
>> Can you guarantee that at the time you allocate the memory using
>> devm_kmalloc() the memory is not dirty (i.e. some write back data are
>> stored in CPU cache) and is not going to be written back in some time,
>> overwriting data put there by IOMMU hardware?
>>
> As I noted in the function "mtk_iommu_hw_init":
>
>        /* protect memory,HW will write here while translation fault */
>        protectpa = __virt_to_phys(piommu->protect_va);
>
>      We don’t care the content of this buffer, It is ok even though its
> data is dirty.
>     It seem to be a the protect memory. While a translation fault
> happened, The iommu HW will overwrite here instead of writing to the
> fault physical address which may be 0 or some random address.
>

Do you mean that it's just a dummy page for hardware behind the IOMMU
to access when the mapping is not available? How would that work with
potential on demand paging when the hardware needs to be blocked until
the mapping is created?

Best regards,
Tomasz
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Yong Wu]

On Wed, 2015-04-15 at 11:20 +0900, Tomasz Figa wrote:
> On Tue, Apr 14, 2015 at 3:31 PM, Yong Wu <yong.wu@mediatek.com> wrote:
> >> >>
> >> >> > +
> >> >> > +       piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,
> >> >>
> >> >> style: Operators like * should have space on both sides.
> >> >>
> >> >> > +                                         GFP_KERNEL);
> >> >>
> >> >> Shouldn't dma_alloc_coherent() be used for this?
> >> >      We don't care the data in it. I think they are the same. Could you
> >> > help tell me why dma_alloc_coherent may be better.
> >>
> >> Can you guarantee that at the time you allocate the memory using
> >> devm_kmalloc() the memory is not dirty (i.e. some write back data are
> >> stored in CPU cache) and is not going to be written back in some time,
> >> overwriting data put there by IOMMU hardware?
> >>
> > As I noted in the function "mtk_iommu_hw_init":
> >
> >        /* protect memory,HW will write here while translation fault */
> >        protectpa = __virt_to_phys(piommu->protect_va);
> >
> >      We don’t care the content of this buffer, It is ok even though its
> > data is dirty.
> >     It seem to be a the protect memory. While a translation fault
> > happened, The iommu HW will overwrite here instead of writing to the
> > fault physical address which may be 0 or some random address.
> >
> 
> Do you mean that it's just a dummy page for hardware behind the IOMMU
> to access when the mapping is not available? How would that work with
> potential on demand paging when the hardware needs to be blocked until
> the mapping is created?
> 
> Best regards,
> Tomasz
 1. YES
 2. Sorry. Our iommu HW can not support this right now. The HW can not
be blocked until the mapping is created.
    If the page is not ready, we can not get the physical address, then
How to fill the pagetable for that memory. I think the dma&iommu may
guaranty it?


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

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Tomasz Figa]

On Wed, Apr 15, 2015 at 4:06 PM, Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> On Wed, 2015-04-15 at 11:20 +0900, Tomasz Figa wrote:
>> On Tue, Apr 14, 2015 at 3:31 PM, Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>> >> >>
>> >> >> > +
>> >> >> > +       piommu->protect_va = devm_kmalloc(piommu->dev, MTK_PROTECT_PA_ALIGN*2,
>> >> >>
>> >> >> style: Operators like * should have space on both sides.
>> >> >>
>> >> >> > +                                         GFP_KERNEL);
>> >> >>
>> >> >> Shouldn't dma_alloc_coherent() be used for this?
>> >> >      We don't care the data in it. I think they are the same. Could you
>> >> > help tell me why dma_alloc_coherent may be better.
>> >>
>> >> Can you guarantee that at the time you allocate the memory using
>> >> devm_kmalloc() the memory is not dirty (i.e. some write back data are
>> >> stored in CPU cache) and is not going to be written back in some time,
>> >> overwriting data put there by IOMMU hardware?
>> >>
>> > As I noted in the function "mtk_iommu_hw_init":
>> >
>> >        /* protect memory,HW will write here while translation fault */
>> >        protectpa = __virt_to_phys(piommu->protect_va);
>> >
>> >      We don’t care the content of this buffer, It is ok even though its
>> > data is dirty.
>> >     It seem to be a the protect memory. While a translation fault
>> > happened, The iommu HW will overwrite here instead of writing to the
>> > fault physical address which may be 0 or some random address.
>> >
>>
>> Do you mean that it's just a dummy page for hardware behind the IOMMU
>> to access when the mapping is not available? How would that work with
>> potential on demand paging when the hardware needs to be blocked until
>> the mapping is created?
>>
>> Best regards,
>> Tomasz
>  1. YES
>  2. Sorry. Our iommu HW can not support this right now. The HW can not
> be blocked until the mapping is created.

OK, that explains it. Well, then I guess this is necessary and
contents of that memory don't matter that much. (Although, this might
be a minor security issue, because the faulting hardware would get
access to some data previously stored by kernel code. Not sure how
much of a threat would that be, though.)

>     If the page is not ready, we can not get the physical address, then
> How to fill the pagetable for that memory. I think the dma&iommu may
> guaranty it?

If your hardware can't block until the mapping is created then what
you do currently seems to be the only option. (+/- the missing cache
maintenance at initialization)

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] iommu/mediatek: Add mt8173 IOMMU driver

[Yong Wu]

Dear Tomasz,
     About a hardcode your comment, please help check below.

Dear Mark,
      I would like to add a item in the dtsi of mtk-iommu. Please also
help have a look.


> > > +static const struct mtk_iommu_port mtk_iommu_mt8173_port[] = {
> > > +       /* port name                m4uid slaveid larbid portid
tfid */
> > > +       /* larb0 */
> > > +       {"M4U_PORT_DISP_OVL0",          0,  0,    0,  0,
MTK_TFID(0, 0)},
> > > +       {"M4U_PORT_DISP_RDMA0",         0,  0,    0,  1,
MTK_TFID(0, 1)},
> > > +       {"M4U_PORT_DISP_WDMA0",         0,  0,    0,  2,
MTK_TFID(0, 2)},
> > > +       {"M4U_PORT_DISP_OD_R",          0,  0,    0,  3,
MTK_TFID(0, 3)},
> > > +       {"M4U_PORT_DISP_OD_W",          0,  0,    0,  4,
MTK_TFID(0, 4)},
> > > +       {"M4U_PORT_MDP_RDMA0",          0,  0,    0,  5,
MTK_TFID(0, 5)},
> > > +       {"M4U_PORT_MDP_WDMA",           0,  0,    0,  6,
MTK_TFID(0, 6)},
> > > +       {"M4U_PORT_MDP_WROT0",          0,  0,    0,  7,
MTK_TFID(0, 7)},
[...]
> 
> > > +};
> > > +
> > 
> > Anyway, is it really necessary to hardcode the SoC specific topology
> > data in this driver? Is there really any use besides of printing
port
> > name? If not, you could just print the values in a way letting you
> > quickly look up in the datasheet, without hardcoding this. Or even
> > better, you could print which devices are attached to the port.
> > 
> a) Printing the port name is for debug. We could not request every
iommu
> user to understand smi&local arbiter. When there is irq, they have to
> look up the iommu's datasheet to find out which port error. if we
print
> it directly, It may be more easily to debug.
> 
> b) In mtk_iommu_config_port, according to this hardcode we can be
easily
> to get out which local arbiter and which port we prepare to config.
> 
> c) If we support different SOCs, we could change this arrays easily.
> 
> > 
     There is no similar code in the others iommu, so I prepare to
delete it, But we really need know which local arbiter and which port we
are going to config(which port will enable iommu)
        so we prepare add a item in the dtsi like this:

	iommu: mmsys_iommu@10205000 {
			compatible = "mediatek,mt8173-iommu";
			<...>
+			larb-portes-nr = <M4U_LARB0_PORT_NR
+					 M4U_LARB1_PORT_NR
+					 M4U_LARB2_PORT_NR
+					 M4U_LARB3_PORT_NR
+					 M4U_LARB4_PORT_NR
+					 M4U_LARB5_PORT_NR>;
			larb = <&larb0 &larb1 &larb2 &larb3 &larb4 &larb5>;
			#iommu-cells = <1>;
		};
	larb-portes-nr : the number of the portes in each local arbiter.
        
	If we have this item, we can get which larb and which port from the
portid in the dtsi of the iommu user.

        And while there is isr, I will print the larb-id and the port-id
instead of the string of the port name.

	The M4U_LARB0_PORT_NR/... will be added in
dt-bindings/iommu/mt8173-iommu-port.h[0]

Dear Mark, 
      As above, if I add this item in the dtsi of iommu, is it ok?


[0]:http://lists.linuxfoundation.org/pipermail/iommu/2015-March/012450.html
 

Re: [PATCH 3/5] dt-bindings: mediatek: Add smi dts binding

[Yong Wu]

Hi Mark,
    Thanks very much for review. 
    About the clock name should be the PoV of _this_ device. Could you
help check below?

On Fri, 2015-03-06 at 11:13 +0000, Mark Rutland wrote:
> On Fri, Mar 06, 2015 at 10:48:18AM +0000, yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> > From: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > 
> > This patch add smi binding document.
> 
> Please move binding documents to the start of the series. It makes
> things far easier to review.
> 
> > 
> > Signed-off-by: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > +++ b/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi.txt
> > @@ -0,0 +1,17 @@
> > +SMI hardware block diagram please help check <bindings/iommu/mediatek,iommu.txt>
> > +
> > +Required properties:
> > +- compatible : must be "mediatek,mediatek,mt8173-smi-larb"
> 
> Double vendor prefix?
> 
> What does "larb" mean? It would be nice for the intorductory paragraph
> in this file to explain.
> 
> > +- reg : the register of each local arbiter
> > +- clocks : the clocks of each local arbiter
> > +- clock-name: larb_sub*(3 clockes at most)
> 
> The names required _must_ be specified here, or clock-names is
> pointless.
> 
> The clock names should be from the PoV of _this_ device (i.e. they
> should be the names of the inputs) not from the PoV of the provider
> (i.e. they should not be the names of the outputs from the provider).
> 
> Mark.
> 
     After we check with our SMI Designer. Every SMI local arbiter need
two clocks, which is called  APB clocks and SMI clock.
     APB clock : Advanced Peripheral Bus Clock. It is the clock for
setting the register of local arbiter.
     SMI clock : Smart Multimedia Interface Clock, It is the clock for
transfering the data and command. 

     And all the local arbiters need the smi common clock, so we
separate it. 

     Then I prepare to design the smi the dtsi like this: 

      smi_common:smi@14022000 {
                 compatible = “mediate, mt8173-smi”;
                 reg = <0 0x14022000 0 0x1000>;
                 clocks = <&mmsys MM_SMI_COMMON>;
                 clocks-names = “smi_common”;
      };

      larb0: larb@14021000 {
                 compatible = “mediate, mt8173-smi-larb”;
                 reg = <0 0x14021000 0 0x1000>;
                 smi = <&smi_common>;
                 clocks = <&mmsys MM_SMI_LARB0>, 
		 	<&mmsys MM_SMI_LARB0>;
                 clocks-names = “apb_clk”, “smi_clk”;
     };

     larb1: larb@16010000 {
                 compatible = “mediate, mt8173-smi-larb”;
                 reg = <0 0x16010000 0 0x1000>;
                 smi = <&smi_common>;
                 clocks = <&vdecsys VDEC_CKEN>, 
			 <&mmsys VDEC_LARB_CKEN>;
                 clocks-names = “apb_clk”, “smi_clk”;
      };
      … 
     In some local arbiter, the source clock of the APB clock and the
SMI clock may be the same, like larb0. so the two clocks are the same.
And they may be different in other local arbiteres, like larb1.      

     If it is designed like this, is it ok?

> > +
> > +Example:
> > +	larb1:larb@16010000 {
> > +	        compatible = "mediatek,mt8173-smi-larb";
> > +		reg = <0 0x16010000 0 0x1000>;
> > +		clocks = <&mmsys MM_SMI_COMMON>,
> > +		        <&vdecsys VDEC_CKEN>,
> > +                        <&vdecsys VDEC_LARB_CKEN>;
> > +		clock-names = "larb_sub0", "larb_sub1", "larb_sub2";
> > +	};
> > -- 
> > 1.8.1.1.dirty
> > 
> > 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] dt-bindings: mediatek: Add smi dts binding

[Mark Rutland]

On Tue, Apr 14, 2015 at 10:07:54AM +0100, Yong Wu wrote:
> Hi Mark,
>     Thanks very much for review. 
>     About the clock name should be the PoV of _this_ device. Could you
> help check below?
> 
> On Fri, 2015-03-06 at 11:13 +0000, Mark Rutland wrote:
> > On Fri, Mar 06, 2015 at 10:48:18AM +0000, yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> > > From: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > 
> > > This patch add smi binding document.
> > 
> > Please move binding documents to the start of the series. It makes
> > things far easier to review.
> > 
> > > 
> > > Signed-off-by: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > +++ b/Documentation/devicetree/bindings/soc/mediatek/mediatek,smi.txt
> > > @@ -0,0 +1,17 @@
> > > +SMI hardware block diagram please help check <bindings/iommu/mediatek,iommu.txt>
> > > +
> > > +Required properties:
> > > +- compatible : must be "mediatek,mediatek,mt8173-smi-larb"
> > 
> > Double vendor prefix?
> > 
> > What does "larb" mean? It would be nice for the intorductory paragraph
> > in this file to explain.
> > 
> > > +- reg : the register of each local arbiter
> > > +- clocks : the clocks of each local arbiter
> > > +- clock-name: larb_sub*(3 clockes at most)
> > 
> > The names required _must_ be specified here, or clock-names is
> > pointless.
> > 
> > The clock names should be from the PoV of _this_ device (i.e. they
> > should be the names of the inputs) not from the PoV of the provider
> > (i.e. they should not be the names of the outputs from the provider).
> > 
> > Mark.
> > 
>      After we check with our SMI Designer. Every SMI local arbiter need
> two clocks, which is called  APB clocks and SMI clock.
>      APB clock : Advanced Peripheral Bus Clock. It is the clock for
> setting the register of local arbiter.
>      SMI clock : Smart Multimedia Interface Clock, It is the clock for
> transfering the data and command. 
> 
>      And all the local arbiters need the smi common clock, so we
> separate it. 
> 
>      Then I prepare to design the smi the dtsi like this: 
> 
>       smi_common:smi@14022000 {
>                  compatible = “mediate, mt8173-smi”;
>                  reg = <0 0x14022000 0 0x1000>;
>                  clocks = <&mmsys MM_SMI_COMMON>;
>                  clocks-names = “smi_common”;
>       };
> 
>       larb0: larb@14021000 {
>                  compatible = “mediate, mt8173-smi-larb”;
>                  reg = <0 0x14021000 0 0x1000>;
>                  smi = <&smi_common>;
>                  clocks = <&mmsys MM_SMI_LARB0>, 
> 		 	<&mmsys MM_SMI_LARB0>;
>                  clocks-names = “apb_clk”, “smi_clk”;
>      };
> 
>      larb1: larb@16010000 {
>                  compatible = “mediate, mt8173-smi-larb”;
>                  reg = <0 0x16010000 0 0x1000>;
>                  smi = <&smi_common>;
>                  clocks = <&vdecsys VDEC_CKEN>, 
> 			 <&mmsys VDEC_LARB_CKEN>;
>                  clocks-names = “apb_clk”, “smi_clk”;
>       };
>       … 
>      In some local arbiter, the source clock of the APB clock and the
> SMI clock may be the same, like larb0. so the two clocks are the same.
> And they may be different in other local arbiteres, like larb1.      
> 
>      If it is designed like this, is it ok?

That looks pretty good; the clocks and names on the larb nodes seem
sensible.

The naming of the "smi_common" clock on the smi_common node looks a bit
odd though. Is that really what the clock input is called?

Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] dt-bindings: mediatek: Add smi dts binding

[Yong Wu]

Hi Mark,

On Tue, 2015-04-14 at 11:06 +0100, Mark Rutland wrote:
> On Tue, Apr 14, 2015 at 10:07:54AM +0100, Yong Wu wrote:
> > Hi Mark,
> >     Thanks very much for review. 
> >     About the clock name should be the PoV of _this_ device. Could you
> > help check below?
> > 
> > On Fri, 2015-03-06 at 11:13 +0000, Mark Rutland wrote:
> > > On Fri, Mar 06, 2015 at 10:48:18AM +0000, yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org wrote:
> > > > From: Yong Wu <yong.wu-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > > > 
> > > > This patch add smi binding document.
> > > 
> > > Please move binding documents to the start of the series. It makes
> > > things far easier to review.
> > >  
> > > What does "larb" mean? It would be nice for the intorductory paragraph
> > > in this file to explain.
> > > 
> > > > +- reg : the register of each local arbiter
> > > > +- clocks : the clocks of each local arbiter
> > > > +- clock-name: larb_sub*(3 clockes at most)
> > > 
> > > The names required _must_ be specified here, or clock-names is
> > > pointless.
> > > 
> > > The clock names should be from the PoV of _this_ device (i.e. they
> > > should be the names of the inputs) not from the PoV of the provider
> > > (i.e. they should not be the names of the outputs from the provider).
> > > 
> > > Mark.
> > > 
> >      After we check with our SMI Designer. Every SMI local arbiter need
> > two clocks, which is called  APB clocks and SMI clock.
> >      APB clock : Advanced Peripheral Bus Clock. It is the clock for
> > setting the register of local arbiter.
> >      SMI clock : Smart Multimedia Interface Clock, It is the clock for
> > transfering the data and command. 
> > 
> >      And all the local arbiters need the smi common clock, so we
> > separate it. 
> > 
> >      Then I prepare to design the smi the dtsi like this: 
> > 
> >       smi_common:smi@14022000 {
> >                  compatible = “mediate, mt8173-smi”;
> >                  reg = <0 0x14022000 0 0x1000>;
> >                  clocks = <&mmsys MM_SMI_COMMON>;
> >                  clocks-names = “smi_common”;
> >       };
> > 
> >       larb0: larb@14021000 {
> >                  compatible = “mediate, mt8173-smi-larb”;
> >                  reg = <0 0x14021000 0 0x1000>;
> >                  smi = <&smi_common>;
> >                  clocks = <&mmsys MM_SMI_LARB0>, 
> > 		 	<&mmsys MM_SMI_LARB0>;
> >                  clocks-names = “apb_clk”, “smi_clk”;
> >      };
> > 
> >      larb1: larb@16010000 {
> >                  compatible = “mediate, mt8173-smi-larb”;
> >                  reg = <0 0x16010000 0 0x1000>;
> >                  smi = <&smi_common>;
> >                  clocks = <&vdecsys VDEC_CKEN>, 
> > 			 <&mmsys VDEC_LARB_CKEN>;
> >                  clocks-names = “apb_clk”, “smi_clk”;
> >       };
> >       … 
> >      In some local arbiter, the source clock of the APB clock and the
> > SMI clock may be the same, like larb0. so the two clocks are the same.
> > And they may be different in other local arbiteres, like larb1.      
> > 
> >      If it is designed like this, is it ok?
> 
> That looks pretty good; the clocks and names on the larb nodes seem
> sensible.
> 
> The naming of the "smi_common" clock on the smi_common node looks a bit
> odd though. Is that really what the clock input is called?
> 
> Mark.
     After check with DE, the smi_common clock also have its APB clock
and the smi clock(they have the same clock source).
    And I prepare to delete "_clk" in all the clock-names.
    So it may be like this:
         smi_common:smi@14022000 {
                 compatible = “mediate, mt8173-smi”;
                    reg = <0 0x14022000 0 0x1000>;
                  clocks = <&mmsys MM_SMI_COMMON>, 
			   <&mmsys MM_SMI_COMMON>;
                  clocks-names = “apk”,"smi";
        };
     How about this?
   




--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/5] dt-bindings: mediatek: Add smi dts binding

[Yong Wu]

On Tue, 2015-04-14 at 21:49 +0800, Yong Wu wrote:
> Hi Mark,
> 
> On Tue, 2015-04-14 at 11:06 +0100, Mark Rutland wrote:
> > On Tue, Apr 14, 2015 at 10:07:54AM +0100, Yong Wu wrote:
> > > Hi Mark,
> > >     Thanks very much for review. 
> > >     About the clock name should be the PoV of _this_ device. Could you
> > > help check below?
> > > 
> > > On Fri, 2015-03-06 at 11:13 +0000, Mark Rutland wrote:
> > > > On Fri, Mar 06, 2015 at 10:48:18AM +0000, yong.wu@mediatek.com wrote:
> > > > > From: Yong Wu <yong.wu@mediatek.com>
> > > > > 
> > > > > This patch add smi binding document.
> > > > 
> > > > Please move binding documents to the start of the series. It makes
> > > > things far easier to review.
> > > >  
> > > > What does "larb" mean? It would be nice for the intorductory paragraph
> > > > in this file to explain.
> > > > 
> > > > > +- reg : the register of each local arbiter
> > > > > +- clocks : the clocks of each local arbiter
> > > > > +- clock-name: larb_sub*(3 clockes at most)
> > > > 
> > > > The names required _must_ be specified here, or clock-names is
> > > > pointless.
> > > > 
> > > > The clock names should be from the PoV of _this_ device (i.e. they
> > > > should be the names of the inputs) not from the PoV of the provider
> > > > (i.e. they should not be the names of the outputs from the provider).
> > > > 
> > > > Mark.
> > > > 
> > >      After we check with our SMI Designer. Every SMI local arbiter need
> > > two clocks, which is called  APB clocks and SMI clock.
> > >      APB clock : Advanced Peripheral Bus Clock. It is the clock for
> > > setting the register of local arbiter.
> > >      SMI clock : Smart Multimedia Interface Clock, It is the clock for
> > > transfering the data and command. 
> > > 
> > >      And all the local arbiters need the smi common clock, so we
> > > separate it. 
> > > 
> > >      Then I prepare to design the smi the dtsi like this: 
> > > 
> > >       smi_common:smi@14022000 {
> > >                  compatible = “mediate, mt8173-smi”;
> > >                  reg = <0 0x14022000 0 0x1000>;
> > >                  clocks = <&mmsys MM_SMI_COMMON>;
> > >                  clocks-names = “smi_common”;
> > >       };
> > > 
> > >       larb0: larb@14021000 {
> > >                  compatible = “mediate, mt8173-smi-larb”;
> > >                  reg = <0 0x14021000 0 0x1000>;
> > >                  smi = <&smi_common>;
> > >                  clocks = <&mmsys MM_SMI_LARB0>, 
> > > 		 	<&mmsys MM_SMI_LARB0>;
> > >                  clocks-names = “apb_clk”, “smi_clk”;
> > >      };
> > > 
> > >      larb1: larb@16010000 {
> > >                  compatible = “mediate, mt8173-smi-larb”;
> > >                  reg = <0 0x16010000 0 0x1000>;
> > >                  smi = <&smi_common>;
> > >                  clocks = <&vdecsys VDEC_CKEN>, 
> > > 			 <&mmsys VDEC_LARB_CKEN>;
> > >                  clocks-names = “apb_clk”, “smi_clk”;
> > >       };
> > >       … 
> > >      In some local arbiter, the source clock of the APB clock and the
> > > SMI clock may be the same, like larb0. so the two clocks are the same.
> > > And they may be different in other local arbiteres, like larb1.      
> > > 
> > >      If it is designed like this, is it ok?
> > 
> > That looks pretty good; the clocks and names on the larb nodes seem
> > sensible.
> > 
> > The naming of the "smi_common" clock on the smi_common node looks a bit
> > odd though. Is that really what the clock input is called?
> > 
> > Mark.
>      After check with DE, the smi_common clock also have its APB clock
> and the smi clock(they have the same clock source).
>     And I prepare to delete "_clk" in all the clock-names.
>     So it may be like this:
>          smi_common:smi@14022000 {
>                  compatible = “mediate, mt8173-smi”;
>                   reg = <0 0x14022000 0 0x1000>;
>                   clocks = <&mmsys MM_SMI_COMMON>, 
> 			   <&mmsys MM_SMI_COMMON>;
>                   clocks-names = “apk”,"smi";
>         };
>      How about this?
      Sorry, I wrote wrong, it should be 
      clock-names = “apb”,"smi";
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek


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

Re: [PATCH 3/5] dt-bindings: mediatek: Add smi dts binding

[Mark Rutland]

> > >       smi_common:smi@14022000 {
> > >                  compatible = “mediate, mt8173-smi”;
> > >                  reg = <0 0x14022000 0 0x1000>;
> > >                  clocks = <&mmsys MM_SMI_COMMON>;
> > >                  clocks-names = “smi_common”;
> > >       };
> > > 
> > >       larb0: larb@14021000 {
> > >                  compatible = “mediate, mt8173-smi-larb”;
> > >                  reg = <0 0x14021000 0 0x1000>;
> > >                  smi = <&smi_common>;
> > >                  clocks = <&mmsys MM_SMI_LARB0>, 
> > > 		 	<&mmsys MM_SMI_LARB0>;
> > >                  clocks-names = “apb_clk”, “smi_clk”;
> > >      };
> > > 
> > >      larb1: larb@16010000 {
> > >                  compatible = “mediate, mt8173-smi-larb”;
> > >                  reg = <0 0x16010000 0 0x1000>;
> > >                  smi = <&smi_common>;
> > >                  clocks = <&vdecsys VDEC_CKEN>, 
> > > 			 <&mmsys VDEC_LARB_CKEN>;
> > >                  clocks-names = “apb_clk”, “smi_clk”;
> > >       };
> > >       … 
> > >      In some local arbiter, the source clock of the APB clock and the
> > > SMI clock may be the same, like larb0. so the two clocks are the same.
> > > And they may be different in other local arbiteres, like larb1.      
> > > 
> > >      If it is designed like this, is it ok?
> > 
> > That looks pretty good; the clocks and names on the larb nodes seem
> > sensible.
> > 
> > The naming of the "smi_common" clock on the smi_common node looks a bit
> > odd though. Is that really what the clock input is called?
> > 
> > Mark.
>      After check with DE, the smi_common clock also have its APB clock
> and the smi clock(they have the same clock source).
>     And I prepare to delete "_clk" in all the clock-names.
>     So it may be like this:
>          smi_common:smi@14022000 {
>                  compatible = “mediate, mt8173-smi”;
>                     reg = <0 0x14022000 0 0x1000>;
>                   clocks = <&mmsys MM_SMI_COMMON>, 
> 			   <&mmsys MM_SMI_COMMON>;
>                   clocks-names = “apk”,"smi";
>         };
>      How about this?

That looks fine to me. I assume "apk" should be "apb" in the last
example.

Mark.