Re: [PATCH] iommu: rockchip: fix building without CONFIG_OF

[JeffyChen]

Hi Amd,

Thanks for your patch.

On 04/04/2018 06:23 PM, Arnd Bergmann wrote:

We get a build error when compiling the iommu driver without CONFIG_OF:

drivers/iommu/rockchip-iommu.c: In function 'rk_iommu_of_xlate':
drivers/iommu/rockchip-iommu.c:1101:2: error: implicit declaration of function 
'of_dev_put'; did you mean 'of_node_put'? 
[-Werror=implicit-function-declaration]


oops, didn't notice that.

and checking other iommu drivers which call of_find_device_by_node() 
too, seems they didn't put() it? maybe we should fix them too



This replaces the of_dev_put() with the equivalent platform_device_put(),
and adds an error check for of_find_device_by_node() returning NULL,
which seems to be appropriate here given that we pass the device into
platform_get_drvdata() next, and that of_find_device_by_node() might
theoretically return a NULL pointer.

hmm, i thought the of_iommu_xlate() checked that before calling us:


static int of_iommu_xlate(struct device *dev,
  struct of_phandle_args *iommu_spec)
{
...
if ((ops && !ops->of_xlate) ||
!of_device_is_available(iommu_spec->np) ||
(!ops && !of_iommu_driver_present(iommu_spec->np)))
return NO_IOMMU;
...
return ops->of_xlate(dev, iommu_spec);


but it's better to check it ourself :)




Fixes: 5fd577c3eac3 ("iommu/rockchip: Use OF_IOMMU to attach devices 
automatically")
Signed-off-by: Arnd Bergmann 
---
This warning appears to only have been introduced in linux-next after
the merge window opened.
---
  drivers/iommu/rockchip-iommu.c | 6 +-
  1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 5fc8656c60f9..fe9c9cc1a9d4 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1094,11 +1094,15 @@ static int rk_iommu_of_xlate(struct device *dev,
return -ENOMEM;

iommu_dev = of_find_device_by_node(args->np);
+   if (!iommu_dev) {
+   kfree(data);
+   return -ENODEV;
+   }

data->iommu = platform_get_drvdata(iommu_dev);
dev->archdata.iommu = data;

-   of_dev_put(iommu_dev);
+   platform_device_put(iommu_dev);

return 0;
  }




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

Re: [PATCH v8 11/14] iommu/rockchip: Use OF_IOMMU to attach devices automatically

[JeffyChen]

Hi Daniel,

Thanks for your reply.

On 03/26/2018 02:31 PM, Daniel Kurtz wrote:

>+struct rk_iommudata {
>+   struct rk_iommu *iommu;
>+};

Why do we need this struct?  Can't we just assign a pointer to struct
rk_iommu directly to dev->archdata.iommu?


hmmm, i was trying to add more device related data in patch[13]:

 struct rk_iommudata {
+   struct device_link *link; /* runtime PM link from IOMMU to master */
struct rk_iommu *iommu;
 };





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

Re: [PATCH v7 13/14] iommu/rockchip: Add runtime PM support

[JeffyChen]

Hi Tomasz,

Thanks for your reply.

On 03/06/2018 06:07 PM, Tomasz Figa wrote:

Hi Jeffy,

It looks like I missed some details of how runtime PM enable works
before, so we might be able to simplify things. Sorry for not getting
things right earlier


hmm, right, the enable state should be the same during those functions. 
will do it in the next version.

.


On Tue, Mar 6, 2018 at 12:27 PM, Jeffy Chen  wrote:

When the power domain is powered off, the IOMMU cannot be accessed and
register programming must be deferred until the power domain becomes
enabled.

Add runtime PM support, and use runtime PM device link from IOMMU to
master to startup and shutdown IOMMU.

Signed-off-by: Jeffy Chen 
---

Changes in v7:
Add WARN_ON in irq isr, and modify iommu archdata comment.

Changes in v6: None
Changes in v5:
Avoid race about pm_runtime_get_if_in_use() and pm_runtime_enabled().

Changes in v4: None
Changes in v3:
Only call startup() and shutdown() when iommu attached.
Remove pm_mutex.
Check runtime PM disabled.
Check pm_runtime in rk_iommu_irq().

Changes in v2: None

  drivers/iommu/rockchip-iommu.c | 189 -
  1 file changed, 148 insertions(+), 41 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 2448a0528e39..db08978203f7 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -22,6 +22,7 @@
  #include 
  #include 
  #include 
+#include 
  #include 
  #include 

@@ -105,7 +106,14 @@ struct rk_iommu {
 struct iommu_domain *domain; /* domain to which iommu is attached */
  };

+/**
+ * struct rk_iommudata - iommu archdata of master device.
+ * @link:  device link with runtime PM integration from the master
+ * (consumer) to the IOMMU (supplier).
+ * @iommu: IOMMU of the master device.
+ */
  struct rk_iommudata {
+   struct device_link *link;
 struct rk_iommu *iommu;
  };

@@ -518,7 +526,13 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id)
 u32 int_status;
 dma_addr_t iova;
 irqreturn_t ret = IRQ_NONE;
-   int i;
+   bool need_runtime_put;
+   int i, err;
+
+   err = pm_runtime_get_if_in_use(iommu->dev);
+   if (WARN_ON(err <= 0 && err != -EINVAL))
+   return ret;
+   need_runtime_put = err > 0;


Actually, for our purposes, we can assume that runtime PM enable
status can be only changed by the driver itself. Looking at the LXR,
PM core also calls __pm_runtime_disable() before calling
.suspend_late() callback and pm_runtime_enable() after calling
.resume_early() callback, but we should be able to ignore this,
because we handle things in .suspend() callback in this driver.

With this assumption in mind, all we need to do here is:

if (WARN_ON(!pm_runtime_get_if_in_use(iommu->dev)))
 return 0;



 WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks));

@@ -570,6 +584,9 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id)

 clk_bulk_disable(iommu->num_clocks, iommu->clocks);

+   if (need_runtime_put)
+   pm_runtime_put(iommu->dev);


if (pm_runtime_enabled())
 pm_runtime_put(iommu->dev);


+
 return ret;
  }

@@ -611,10 +628,20 @@ static void rk_iommu_zap_iova(struct rk_iommu_domain 
*rk_domain,
 spin_lock_irqsave(_domain->iommus_lock, flags);
 list_for_each(pos, _domain->iommus) {
 struct rk_iommu *iommu;
+   int ret;
+
 iommu = list_entry(pos, struct rk_iommu, node);
-   WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks));
-   rk_iommu_zap_lines(iommu, iova, size);
-   clk_bulk_disable(iommu->num_clocks, iommu->clocks);
+
+   /* Only zap TLBs of IOMMUs that are powered on. */
+   ret = pm_runtime_get_if_in_use(iommu->dev);
+   if (ret > 0 || ret == -EINVAL) {


if (pm_runtime_get_if_in_use(iommu->dev)) {


+   WARN_ON(clk_bulk_enable(iommu->num_clocks,
+   iommu->clocks));
+   rk_iommu_zap_lines(iommu, iova, size);
+   clk_bulk_disable(iommu->num_clocks, iommu->clocks);


 if (pm_runtime_enabled(iommu->dev))
 pm_runtime_put(iommu->dev);

And so on, in other places.

Really sorry for the confusion before.

Best regards,
Tomasz






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

Re: [RESEND PATCH v6 09/14] dt-bindings: iommu/rockchip: Add clock property

[JeffyChen]

Hi Rob,

Thanks for your reply.

On 03/06/2018 10:27 AM, Rob Herring wrote:

On Thu, Mar 01, 2018 at 06:18:32PM +0800, Jeffy Chen wrote:

Add clock property, since we are going to control clocks in rockchip
iommu driver.

Signed-off-by: Jeffy Chen 
---

Changes in v6: None


Really? There was a different number of clocks before.

hmm, right, forgot to modify the change log, sorry...



Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

  Documentation/devicetree/bindings/iommu/rockchip,iommu.txt | 7 +++
  1 file changed, 7 insertions(+)


Reviewed-by: Rob Herring 






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

Re: [RESEND PATCH v6 13/14] iommu/rockchip: Add runtime PM support

[JeffyChen]

Hi Tomasz,

Thanks for your reply.

On 03/05/2018 09:49 PM, Tomasz Figa wrote:

>  struct rk_iommudata {
>+   struct device_link *link; /* runtime PM link from IOMMU to master */

Kerneldoc comment for the struct could be added instead.

i saw this in the kerneldoc:

* An MMU device exists alongside a busmaster device, both are in the same
  power domain.  The MMU implements DMA address translation for the 
busmaster

  device and shall be runtime resumed and kept active whenever and as long
  as the busmaster device is active.  The busmaster device's driver shall
  not bind before the MMU is bound.  To achieve this, a device link with
  runtime PM integration is added from the busmaster device (consumer)
  to the MMU device (supplier).  The effect with regards to runtime PM
  is the same as if the MMU was the parent of the master device.


maybe we can use something like:
device link with runtime PM integration from the master (consumer) to 
the IOMMU (supplier).





> struct rk_iommu *iommu;
>  };
>
>@@ -518,7 +520,12 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id)
> u32 int_status;
> dma_addr_t iova;
> irqreturn_t ret = IRQ_NONE;
>-   int i;
>+   int i, err, need_runtime_put;

nit: need_runtime_put could be a bool.

ok




>+
>+   err = pm_runtime_get_if_in_use(iommu->dev);
>+   if (err <= 0 && err != -EINVAL)
>+   return ret;
>+   need_runtime_put = err > 0;

Generally something must be really wrong if we end up with err == 0
here, because the IOMMU must be powered on to signal an interrupt. The
only case this could happen would be if the IRQ signal was shared with
some device from another power domain. Is it possible on Rockchip
SoCs? If not, perhaps we should have a WARN_ON() here for such case.
the irq could be shared between master and IOMMU, but always from the 
same power domain i think.


will add a WARN_ON()



>
> WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks));
>
>@@ -570,6 +577,9 @@ static irqreturn_t rk_iommu_irq(int irq, void *dev_id)
>
> clk_bulk_disable(iommu->num_clocks, iommu->clocks);
>
>+   if (need_runtime_put)
>+   pm_runtime_put(iommu->dev);
>+
> return ret;
>  }
>
>@@ -611,10 +621,20 @@ static void rk_iommu_zap_iova(struct rk_iommu_domain 
*rk_domain,
> spin_lock_irqsave(_domain->iommus_lock, flags);
> list_for_each(pos, _domain->iommus) {
> struct rk_iommu *iommu;
>+   int ret;
>+
> iommu = list_entry(pos, struct rk_iommu, node);
>-   WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks));
>-   rk_iommu_zap_lines(iommu, iova, size);
>-   clk_bulk_disable(iommu->num_clocks, iommu->clocks);
>+
>+   /* Only zap TLBs of IOMMUs that are powered on. */
>+   ret = pm_runtime_get_if_in_use(iommu->dev);
>+   if (ret > 0 || ret == -EINVAL) {
>+   WARN_ON(clk_bulk_enable(iommu->num_clocks,
>+   iommu->clocks));
>+   rk_iommu_zap_lines(iommu, iova, size);
>+   clk_bulk_disable(iommu->num_clocks, iommu->clocks);
>+   }
>+   if (ret > 0)
>+   pm_runtime_put(iommu->dev);
> }
> spin_unlock_irqrestore(_domain->iommus_lock, flags);
>  }
>@@ -817,22 +837,30 @@ static struct rk_iommu *rk_iommu_from_dev(struct device 
*dev)
> return data ? data->iommu : NULL;
>  }
>
>-static int rk_iommu_attach_device(struct iommu_domain *domain,
>- struct device *dev)
>+/* Must be called with iommu powered on and attached */
>+static void rk_iommu_shutdown(struct rk_iommu *iommu)
>  {
>-   struct rk_iommu *iommu;
>+   int i;
>+
>+   /* Ignore error while disabling, just keep going */
>+   WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks));
>+   rk_iommu_enable_stall(iommu);
>+   rk_iommu_disable_paging(iommu);
>+   for (i = 0; i < iommu->num_mmu; i++) {
>+   rk_iommu_write(iommu->bases[i], RK_MMU_INT_MASK, 0);
>+   rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR, 0);
>+   }
>+   rk_iommu_disable_stall(iommu);
>+   clk_bulk_disable(iommu->num_clocks, iommu->clocks);
>+}
>+
>+/* Must be called with iommu powered on and attached */
>+static int rk_iommu_startup(struct rk_iommu *iommu)
>+{
>+   struct iommu_domain *domain = iommu->domain;
> struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
>-   unsigned long flags;
> int ret, i;
>
>-   /*
>-* Allow 'virtual devices' (e.g., drm) to attach to domain.
>-* Such a device does not belong to an iommu group.
>-*/
>-   iommu = rk_iommu_from_dev(dev);
>-   if (!iommu)
>-   return 0;
>-
> ret = clk_bulk_enable(iommu->num_clocks, iommu->clocks);
> if 

Re: [RESEND PATCH v6 14/14] iommu/rockchip: Support sharing IOMMU between masters

[JeffyChen]

Hi Robin,

On 03/01/2018 07:03 PM, Robin Murphy wrote:


+static struct iommu_group *rk_iommu_device_group(struct device *dev)
+{
+struct rk_iommu *iommu;
+
+iommu = rk_iommu_from_dev(dev);
+
+return iommu->group;


Oops, seems I overlooked this in my previous review - it should really be:

 return iommu_group_get(iommu->group);

or the refcounting will be unbalanced on those future systems where it
really will be called more than once.


hmmm, right, it should be return iommu_group_ref_get(iommu->group);

i was following omap iommu:
static struct iommu_group *omap_iommu_device_group(struct device *dev)
{
struct omap_iommu_arch_data *arch_data = dev->archdata.iommu;
struct iommu_group *group = ERR_PTR(-EINVAL);

if (arch_data->iommu_dev)
group = arch_data->iommu_dev->group;

return group;
}

will fix it too in the version.


Robin.



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

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

[JeffyChen]

Hi Robin,

Thanks for your reply.

On 02/28/2018 11:06 PM, Robin Murphy wrote:

On 28/02/18 13:00, JeffyChen wrote:

Hi Robin,

Thanks for your reply.

On 02/28/2018 12:59 AM, Robin Murphy wrote:

the rockchip IOMMU is part of the master block in hardware, so it
needs
to control the master's power domain and some of the master's clocks
when access it's registers.

and the number of clocks needed here, might be different between each
IOMMUs(according to which master block it belongs), it's a little like
our power domain:
https://elixir.free-electrons.com/linux/latest/source/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L935




i'm not sure how to describe this correctly, is it ok use something
like
"the same as it's master block"?


would it make sense to add a property to specify the master who owns
the iommu, and we can get all clocks(only some of those clocks are
actually needed) from it in the of_xlate()? and we can also reuse the
clock-names of that master to build clk_bulk_data and log errors in
clk_bulk_get.


I'm inclined to agree with Rob here - if we're to add anything to the
binding, it should only be whatever clock inputs are defined for the
IOMMU IP block itself. If Linux doesn't properly handle the interconnect
clock hierarchy external to a particular integration, that's a separate
issue and it's not the binding's problem.

I actually quite like the hack of "borrowing" the clocks from
dev->of_node in of_xlate() - you shouldn't need any DT changes for that,
because you already know that each IOMMU instance only has the one
master device anyway.


Thanks:) but actually we are going to support sharing IOMMU between
multiple masters(one of them is the main master i think) in the newer
chips(not yet supported on upstream kernel)...


Ha! OK, fair enough, back to the first point then...


So we might have to get all clocks from all masters, or find a way to
specify the main master...and for the multiple masters case, do it in
of_xlate() turns out to be a little racy...maybe we can add a property
to specify main master, and get it's clocks in probe()?


I notice that the 4.4 BSP kernel consistently specifies "aclk" and
"hclk" for the IOMMU instances - it feels unusual to say "why don't we
follow the downstream binding?", but it does look a lot like what I
would expect (I'd guess at one for the register slave interface and one
for the master interface/general operation?)

huh, right.
i did noticed that, but there's a hevc_mmu with ("aclk", "hclk", 
"clk_core", "clk_cabac") confused me.


so confirmed with Simon, that hevc_mmu is wrong. currently all IOMMUs 
should only have 2 clks, either aclk+hclk or aclk+pclk (depends on the 
clk tree)


so it seems to be a good idea to do so, will send patches soon, thanks :)


If we can implement conceptually-correct clock handling based on an
accurate binding, which should cover most cases, and *then* look at
hacking around those where it doesn't quite work in practice due to
shortcomings elsewhere, that would be ideal, and of course a lot nicer
than just jumping straight into piles of hacks.

Robin.






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

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

[JeffyChen]

Hi Robin,

Thanks for your reply.

On 02/28/2018 12:59 AM, Robin Murphy wrote:

the rockchip IOMMU is part of the master block in hardware, so it needs
to control the master's power domain and some of the master's clocks
when access it's registers.

and the number of clocks needed here, might be different between each
IOMMUs(according to which master block it belongs), it's a little like
our power domain:
https://elixir.free-electrons.com/linux/latest/source/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L935



i'm not sure how to describe this correctly, is it ok use something like
"the same as it's master block"?


would it make sense to add a property to specify the master who owns
the iommu, and we can get all clocks(only some of those clocks are
actually needed) from it in the of_xlate()? and we can also reuse the
clock-names of that master to build clk_bulk_data and log errors in
clk_bulk_get.


I'm inclined to agree with Rob here - if we're to add anything to the
binding, it should only be whatever clock inputs are defined for the
IOMMU IP block itself. If Linux doesn't properly handle the interconnect
clock hierarchy external to a particular integration, that's a separate
issue and it's not the binding's problem.

I actually quite like the hack of "borrowing" the clocks from
dev->of_node in of_xlate() - you shouldn't need any DT changes for that,
because you already know that each IOMMU instance only has the one
master device anyway.


Thanks:) but actually we are going to support sharing IOMMU between 
multiple masters(one of them is the main master i think) in the newer 
chips(not yet supported on upstream kernel)...


So we might have to get all clocks from all masters, or find a way to 
specify the main master...and for the multiple masters case, do it in 
of_xlate() turns out to be a little racy...maybe we can add a property 
to specify main master, and get it's clocks in probe()?




Robin.



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

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

[JeffyChen]

Hi guys,

On 01/25/2018 06:24 PM, JeffyChen wrote:

On 01/25/2018 05:42 PM, Randy Li wrote:




confirmed with Simon, there might be some iommus don't have a pd, and

We use the pd to control the NIU node(not on upstream), without a pd or
fake pd, none of the platform would work.

after talked offline, it's possible to have iommu without pd in upstream
kernel(and chromeos kernel), but on our internal kernel, the drivers
would require pd(or fake pd) to reset modules when error happens.

anyway, i think that means we do need clock control here.


found another reason to not depend on pd to control clocks.

currently we are using pd's pm_clk to keep clocks enabled during power 
on. but in our pd binding doc, that is not needed:
- clocks (optional): phandles to clocks which need to be enabled while 
power domain

switches state.

confirmed with Caesar, the pm_clk only required for some old 
chips(rk3288 for example) due to hardware issue.


and i tested my chromebook kevin(rk3399), it works well after remove the 
pm_clk:


+++ b/drivers/soc/rockchip/pm_domains.c
@@ -478,7 +478,6 @@ static int rockchip_pm_add_one_domain(struct 
rockchip_pmu *pmu,

pd->genpd.power_on = rockchip_pd_power_on;
pd->genpd.attach_dev = rockchip_pd_attach_dev;
pd->genpd.detach_dev = rockchip_pd_detach_dev;
-   pd->genpd.flags = GENPD_FLAG_PM_CLK;

will do more tests and send patch tomorrow.




the CONFIG_PM could be disabled.I am hard to believe a modern platform
can work without that.

so it might be better to control clocks in iommu driver itself.

I won't
insist how the version of the iommu patch on the upstream, I
just post an idea here.
The version for kernel 4.4 is under internal review, the implementation
has been modified many times.

I would suggest the managing clocks in pd is a more easy way and don't
need to spare those thing in two places.



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

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

[JeffyChen]

Hi guys,

On 02/01/2018 07:19 PM, JeffyChen wrote:



diff --git
a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
index 2098f7732264..33dd853359fa 100644
--- a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
@@ -14,6 +14,13 @@ Required properties:
  "single-master" device, and needs no
additional information
  to associate with its master device.  See:

Documentation/devicetree/bindings/iommu/iommu.txt
+Optional properties:
+- clocks : A list of master clocks requires for the IOMMU to be
accessible
+   by the host CPU. The number of clocks depends on the
master
+   block and might as well be zero. See [1] for generic clock
+   bindings description.


Hardware blocks don't have a variable number of clock connections.


I think you underestimate the imagination of hardware designers. :)


Learned long ago to never do that. If there are 2 ways to do
something, they will find a 3rd way.


For Rockchip IOMMU, there is a set of clocks, which all need to be
enabled for IOMMU register access to succeed. The clocks are not
directly fed to the IOMMU, but they are needed for the various buses
and intermediate blocks on the way to the IOMMU to work.


The binding should describe the clock connections, not what clocks a
driver needs (currently). It sounds like a lack of managing bus clocks
to me.

In any case, the binding must be written so it can be verified. If you
can have any number of clocks with any names, there's no point in
documenting.


the rockchip IOMMU is part of the master block in hardware, so it needs
to control the master's power domain and some of the master's clocks
when access it's registers.

and the number of clocks needed here, might be different between each
IOMMUs(according to which master block it belongs), it's a little like
our power domain:
https://elixir.free-electrons.com/linux/latest/source/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L935


i'm not sure how to describe this correctly, is it ok use something like
"the same as it's master block"?


would it make sense to add a property to specify the master who owns the 
iommu, and we can get all clocks(only some of those clocks are actually 
needed) from it in the of_xlate()? and we can also reuse the clock-names 
of that master to build clk_bulk_data and log errors in clk_bulk_get.






Rob



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

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

[JeffyChen]

Hi Rob,

Thanks for your reply.

On 01/31/2018 09:50 PM, Rob Herring wrote:

On Wed, Jan 31, 2018 at 1:52 AM, Tomasz Figa  wrote:

Hi Rob,

On Wed, Jan 31, 2018 at 2:05 AM, Rob Herring  wrote:

On Wed, Jan 24, 2018 at 06:35:11PM +0800, Jeffy Chen wrote:

From: Tomasz Figa 

Current code relies on master driver enabling necessary clocks before
IOMMU is accessed, however there are cases when the IOMMU should be
accessed while the master is not running yet, for example allocating
V4L2 videobuf2 buffers, which is done by the VB2 framework using DMA
mapping API and doesn't engage the master driver at all.

This patch fixes the problem by letting clocks needed for IOMMU
operation to be listed in Device Tree and making the driver enable them
for the time of accessing the hardware.

Signed-off-by: Jeffy Chen 
Signed-off-by: Tomasz Figa 
---

Changes in v5:
Use clk_bulk APIs.

Changes in v4: None
Changes in v3: None
Changes in v2: None

  .../devicetree/bindings/iommu/rockchip,iommu.txt   |  8 +++


Please split binding patches to a separate patch.

right, i'll do that in the next version.



  drivers/iommu/rockchip-iommu.c | 74 --
  2 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt 
b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
index 2098f7732264..33dd853359fa 100644
--- a/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/rockchip,iommu.txt
@@ -14,6 +14,13 @@ Required properties:
  "single-master" device, and needs no additional 
information
  to associate with its master device.  See:
  Documentation/devicetree/bindings/iommu/iommu.txt
+Optional properties:
+- clocks : A list of master clocks requires for the IOMMU to be accessible
+   by the host CPU. The number of clocks depends on the master
+   block and might as well be zero. See [1] for generic clock
+   bindings description.


Hardware blocks don't have a variable number of clock connections.


I think you underestimate the imagination of hardware designers. :)


Learned long ago to never do that. If there are 2 ways to do
something, they will find a 3rd way.


For Rockchip IOMMU, there is a set of clocks, which all need to be
enabled for IOMMU register access to succeed. The clocks are not
directly fed to the IOMMU, but they are needed for the various buses
and intermediate blocks on the way to the IOMMU to work.


The binding should describe the clock connections, not what clocks a
driver needs (currently). It sounds like a lack of managing bus clocks
to me.

In any case, the binding must be written so it can be verified. If you
can have any number of clocks with any names, there's no point in
documenting.


the rockchip IOMMU is part of the master block in hardware, so it needs 
to control the master's power domain and some of the master's clocks 
when access it's registers.


and the number of clocks needed here, might be different between each 
IOMMUs(according to which master block it belongs), it's a little like 
our power domain:

https://elixir.free-electrons.com/linux/latest/source/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L935

i'm not sure how to describe this correctly, is it ok use something like 
"the same as it's master block"?




Rob






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

Re: [PATCH v5 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

[JeffyChen]

Hi Robin,

Thanks for your reply.

On 01/24/2018 09:49 PM, Robin Murphy wrote:


+Optional properties:
+- clocks : A list of master clocks requires for the IOMMU to be
accessible


s/requires/required/

ok



+   by the host CPU. The number of clocks depends on the master
+   block and might as well be zero. See [1] for generic clock


Oops, some subtleties of English here :)

To say "the number of clocks ... might as well be zero" effectively
implies "there's no point ever specifying any clocks". I guess what you
really mean here is "...might well be...", i.e. it is both valid and
reasonably likely to require zero clocks.

ok



+   bindings description.
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
  Optional properties:
  - rockchip,disable-mmu-reset : Don't use the mmu reset operation.
@@ -27,5 +34,6 @@ Example:
  reg = <0xff940300 0x100>;
  interrupts = ;
  interrupt-names = "vopl_mmu";
+clocks = < ACLK_VOP1>, < DCLK_VOP1>, < HCLK_VOP1>;
  #iommu-cells = <0>;
  };
diff --git a/drivers/iommu/rockchip-iommu.c
b/drivers/iommu/rockchip-iommu.c
index c4131ca792e0..8a5e2a659b67 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -4,6 +4,7 @@
   * published by the Free Software Foundation.
   */
+#include 
  #include 
  #include 
  #include 
@@ -91,6 +92,8 @@ struct rk_iommu {
  struct device *dev;
  void __iomem **bases;
  int num_mmu;
+struct clk_bulk_data *clocks;
+int num_clocks;
  bool reset_disabled;
  struct iommu_device iommu;
  struct list_head node; /* entry in rk_iommu_domain.iommus */
@@ -450,6 +453,38 @@ static int rk_iommu_force_reset(struct rk_iommu
*iommu)
  return 0;
  }
+static int rk_iommu_of_get_clocks(struct rk_iommu *iommu)
+{
+struct device_node *np = iommu->dev->of_node;
+int ret;
+int i;
+
+ret = of_count_phandle_with_args(np, "clocks", "#clock-cells");
+if (ret == -ENOENT)
+return 0;
+else if (ret < 0)
+return ret;
+
+iommu->num_clocks = ret;
+iommu->clocks = devm_kcalloc(iommu->dev, iommu->num_clocks,
+ sizeof(*iommu->clocks), GFP_KERNEL);
+if (!iommu->clocks)
+return -ENOMEM;
+
+for (i = 0; i < iommu->num_clocks; ++i) {
+iommu->clocks[i].clk = of_clk_get(np, i);
+if (IS_ERR(iommu->clocks[i].clk)) {
+ret = PTR_ERR(iommu->clocks[i].clk);
+goto err_clk_put;
+}
+}


Just to confirm my understanding from a quick scan through the code, the
reason we can't use clk_bulk_get() here is that currently, clocks[i].id
being NULL means we'd end up just getting the first clock multiple
times, right?

right, without a valid name, it would return the first clock.

/* Walk up the tree of devices looking for a clock that matches */
while (np) {
int index = 0;

/*
 * For named clocks, first look up the name in the
 * "clock-names" property.  If it cannot be found, then
 * index will be an error code, and of_clk_get() will fail.
 */
if (name)
index = of_property_match_string(np, "clock-names", name);
clk = __of_clk_get(np, index, dev_id, name);




I guess there could be other users who also want "just get whatever
clocks I have" functionality, so it might be worth proposing that for
the core API as a separate/follow-up patch, but it definitely doesn't
need to be part of this series.

right, i can try to do it later :)


I really don't know enough about correct clk API usage, but modulo the
binding comments it certainly looks nice and tidy now;

Acked-by: Robin Murphy 

thanks.


Thanks,
Robin.



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

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

[JeffyChen]

Hi Randy,

On 01/22/2018 10:15 AM, JeffyChen wrote:

Hi Randy,

On 01/22/2018 09:18 AM, Randy Li wrote:



Also the power domain driver could manage the clocks as well, I would
suggest to use pm_runtime_*.


actually the clocks required by pm domain may not be the same as what we
want to control here, there might be some clocks only be needed when
accessing mmu registers.

but i'm not very sure about that, will confirm it with Simon Xue.


confirmed with Simon, there might be some iommus don't have a pd, and 
the CONFIG_PM could be disabled.


so it might be better to control clocks in iommu driver itself.

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

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

[JeffyChen]

Hi Randy,

On 01/22/2018 09:18 AM, Randy Li wrote:



Also the power domain driver could manage the clocks as well, I would
suggest to use pm_runtime_*.


actually the clocks required by pm domain may not be the same as what we 
want to control here, there might be some clocks only be needed when 
accessing mmu registers.


but i'm not very sure about that, will confirm it with Simon Xue.

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

Re: [PATCH v4 07/13] ARM: dts: rockchip: add clocks in vop iommu nodes

[JeffyChen]

Hi Tomasz,

Thanks for your reply.

On 01/19/2018 11:23 AM, Tomasz Figa wrote:

On Thu, Jan 18, 2018 at 8:52 PM, Jeffy Chen  wrote:

Add clocks in vop iommu nodes, since we are going to control clocks in
rockchip iommu driver.

Signed-off-by: Jeffy Chen 
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

  arch/arm/boot/dts/rk3036.dtsi | 2 ++
  arch/arm/boot/dts/rk3288.dtsi | 4 
  2 files changed, 6 insertions(+)

diff --git a/arch/arm/boot/dts/rk3036.dtsi b/arch/arm/boot/dts/rk3036.dtsi
index 3b704cfed69a..95b0ebc7a40f 100644
--- a/arch/arm/boot/dts/rk3036.dtsi
+++ b/arch/arm/boot/dts/rk3036.dtsi
@@ -197,6 +197,8 @@
 reg = <0x10118300 0x100>;
 interrupts = ;
 interrupt-names = "vop_mmu";
+   clocks = < ACLK_LCDC>, < SCLK_LCDC>, < HCLK_LCDC>;
+   clock-names = "aclk_vop", "dclk_vop", "hclk_vop";


We should remove clock-names from IOMMU nodes. The Rockchip IOMMU
bindings don't define clock names and only the clocks property should
be given.

hmmm, i'm trying to switch to clk_bulk APIs, the get and put are name 
based. or maybe i can use clk_get/put along with other clk_bulk APIs

Not even saying that the names currently listed are not good examples,
they name SoC clock controller output, rather than device inputs.

Best regards,
Tomasz






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

Re: [PATCH] iommu/of: Only do IOMMU lookup for available ones

[JeffyChen]

Hi Will,

Thanks for your reply.

On 01/18/2018 10:41 PM, Will Deacon wrote:

>
>Makes sense to me, but I'd like to have an OK from Robin or Will (added
>to Cc) before applying this.

I don't think this patch makes a lot of sense in isolation: the SMMU drivers
themselves will likely still probe, and it's unclear what we should about
DMA when an IOMMU is not deemed to be available. See:

https://patchwork.kernel.org/patch/9681211/

Jeffy -- are you solving a real issue here, or is this just an attempt at
some cleanup?


hmmm, it's just a cleanup, but it seems like we already removed this 
of_iommu_init_fn in:

b0c560f7d8a4 iommu: Clean up of_iommu_init_fn

so this patch can be disregarded too :)


Will






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

Re: [PATCH v4 08/13] iommu/rockchip: Control clocks needed to access the IOMMU

[JeffyChen]

Hi Robin,

On 01/18/2018 08:27 PM, Robin Murphy wrote:




Is it worth using the clk_bulk_*() APIs for this? At a glance, most of
the code being added here appears to duplicate what those functions
already do (but I'm no clk API expert, for sure).
right, i think it's doable, the clk_bulk APIs are very helpful. i think 
we didn't use that is because this patch were wrote for the chromeos 4.4 
kernel, which doesn't have clk_bulk yet:)


will do it in the next version, thanks.


Robin.



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

Re: [PATCH v4 04/13] iommu/rockchip: Fix error handling in attach

[JeffyChen]

Hi Robin,

On 01/18/2018 09:23 PM, Robin Murphy wrote:


@@ -837,7 +837,7 @@ static int rk_iommu_attach_device(struct
iommu_domain *domain,
  ret = rk_iommu_enable_paging(iommu);
  if (ret)
-return ret;
+goto err_disable_stall;
  spin_lock_irqsave(_domain->iommus_lock, flags);
  list_add_tail(>node, _domain->iommus);
@@ -848,6 +848,11 @@ static int rk_iommu_attach_device(struct
iommu_domain *domain,
  rk_iommu_disable_stall(iommu);
  return 0;


Nit: if you like, it looks reasonable to name the label
"out_disable_stall" and remove these lines above here, to save the
duplication between the error and success paths (since ret will already
be 0 on the latter).


right, i think so, will do it in the next version.

Either way,

Reviewed-by: Robin Murphy 



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

Re: [PATCH v4 05/13] iommu/rockchip: Use iopoll helpers to wait for hardware

[JeffyChen]

Hi Robin,

On 01/18/2018 09:09 PM, Robin Murphy wrote:

-#define FORCE_RESET_TIMEOUT100/* ms */
+#define FORCE_RESET_TIMEOUT10/* us */
+#define POLL_TIMEOUT1000/* us */


Nit: the callsites look a bit odd with the combination of POLL_TIMEOUT
and the magic number 100 - should we not also define something like
POLL_PERIOD for that? Also POLL_TIMEOUT is a rather generic name and
overlaps with several other drivers, so a namespace prefix would be
helpful (i.e. RK_IOMMU_POLL*, or at least RK_POLL*).

FWIW, my personal preference would be to also suffix these with _US for
absolute clarity, but it's not essential (especially if longer names
lead to more linebreaks at the callsites).

right, that make sense, will fix it in next version, thanks :)


With those undocumented "100"s fixed up,

Reviewed-by: Robin Murphy 



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

Re: [PATCH v4 00/13] iommu/rockchip: Use OF_IOMMU

[JeffyChen]

Hi Joerg,

Thanks for your reply.

On 01/18/2018 08:44 PM, Joerg Roedel wrote:

On Thu, Jan 18, 2018 at 07:52:38PM +0800, Jeffy Chen wrote:

Jeffy Chen (9):
   iommu/rockchip: Prohibit unbind and remove
   iommu/rockchip: Fix error handling in probe
   iommu/rockchip: Request irqs in rk_iommu_probe()
   ARM: dts: rockchip: add clocks in vop iommu nodes
   iommu/rockchip: Use IOMMU device for dma mapping operations
   iommu/rockchip: Use OF_IOMMU to attach devices automatically
   iommu/rockchip: Fix error handling in init
   iommu/rockchip: Add runtime PM support
   iommu/rockchip: Support sharing IOMMU between masters

Tomasz Figa (4):
   iommu/rockchip: Fix error handling in attach
   iommu/rockchip: Use iopoll helpers to wait for hardware
   iommu/rockchip: Fix TLB flush of secondary IOMMUs
   iommu/rockchip: Control clocks needed to access the IOMMU


Please stop resending this every day with minimal changes. I am not
going to take it for v4.16, as we are late in the cycle already and there
are still review comments.

Just collect all feedback, update the patches, rebase them to v4.16-rc1
when its out, and send a new version.


oops, sorry, i send v4 today is mainly because i found v3 would break 
some of the rk platforms, which needs an extra patch [7] (ARM: dts: 
rockchip: add clocks in vop iommu nodes),  but forgot to mention it in 
the change log...


will send new version after all the rest patches been reviewed :)



Joerg






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

Re: [PATCH v3 01/12] iommu/rockchip: Prohibiat unbind and remove

[JeffyChen]

Hi Tomasz,

Thanks for your reply.

and just found i forgot to add iommu clocks for other rockchip 
platforms(rk3399 already has that)...will also do that in the next version.


On 01/18/2018 12:17 PM, Tomasz Figa wrote:

On Thu, Jan 18, 2018 at 12:25 AM, Jeffy Chen  wrote:

Removal the IOMMUs cannot be done reliably.


nit: Perhaps "Removal of IOMMUs"?


ok


This is similar to exynos iommu driver.

Signed-off-by: Jeffy Chen 
---

Changes in v3:
Also remove remove() and module_exit() as Tomasz suggested.

Changes in v2: None

  drivers/iommu/rockchip-iommu.c | 21 +
  1 file changed, 1 insertion(+), 20 deletions(-)


nit: Typo in subject: s/Prohibiat/Prohibit/


ok, will do that.

Otherwise,

Reviewed-by: Tomasz Figa 

Best regards,
Tomasz






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

Re: [PATCH v2 13/13] iommu/rockchip: Support sharing IOMMU between masters

[JeffyChen]

Hi Robin,

On 01/17/2018 09:00 PM, Robin Murphy wrote:

On 16/01/18 13:25, Jeffy Chen wrote:

There would be some masters sharing the same IOMMU device. Put them in
the same iommu group and share the same iommu domain.

Signed-off-by: Jeffy Chen 
---

Changes in v2: None

  drivers/iommu/rockchip-iommu.c | 39
+++
  1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c
b/drivers/iommu/rockchip-iommu.c
index c8de1456a016..6c316dd0dd2d 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -100,11 +100,13 @@ struct rk_iommu {
  struct iommu_device iommu;
  struct list_head node; /* entry in rk_iommu_domain.iommus */
  struct iommu_domain *domain; /* domain to which iommu is
attached */
+struct iommu_group *group;
  struct mutex pm_mutex; /* serializes power transitions */
  };
  struct rk_iommudata {
  struct device_link *link; /* runtime PM link from IOMMU to
master */
+struct iommu_domain *domain; /* domain to which device is
attached */


I don't see why this is needed - for example, mtk_iommu does the same
thing without needing to track the active domain in more than one place.

Fundamentally, for this kind of IOMMU without the notion of multiple
translation contexts, the logic should look like:

 iommudrv_attach_device(dev, domain) {
 iommu = dev_get_iommu(dev);
 if (iommu->curr_domain != domain) {
 update_hw_state(iommu, domain);
 iommu->curr_domain = domain;
 }
 }

which I think is essentially what you have anyway. When a group contains
multiple devices, you update the IOMMU state for the first device, then
calls for subsequent devices in the group do nothing since they see the
IOMMU state is already up-to-date with the new domain.


right, will remove it.

Robin.



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

Re: [PATCH v2 09/13] iommu/rockchip: Use iommu_group_get_for_dev() for add_device

[JeffyChen]

On 01/17/2018 08:47 PM, JeffyChen wrote:


+static struct iommu_group *rk_iommu_device_group(struct device *dev)
+{
+struct iommu_group *group;
+int ret;
+
+group = iommu_group_get(dev);
+if (!group) {


This check is pointless - if dev->iommu_group were non-NULL you wouldn't
have been called in the first place.

right, it's allocated in the probe.
oops, sorry, i see, this is not needed because device_group() is only be 
called when iommu_group_get() returns NULL


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

Re: [PATCH v2 09/13] iommu/rockchip: Use iommu_group_get_for_dev() for add_device

[JeffyChen]

Hi Robin,

On 01/17/2018 08:31 PM, Robin Murphy wrote:

On 16/01/18 13:25, Jeffy Chen wrote:

IOMMU drivers are supposed to call this function instead of manually
creating a group in their .add_device callback. This behavior is not
strictly required by ARM DMA mapping implementation, but ARM64 already
relies on it. This patch fixes the rockchip-iommu driver to comply with
this requirement.


FWIW that's not 100% true: what arm64 relies on is the group having a
default DMA ops domain. Technically, you *could* open-code that in the
driver's group allocation, but obviously using the appropriate existing
API is nicer :)

ok, will rewrite the commit message.


[...]

@@ -1182,6 +1164,29 @@ static void rk_iommu_remove_device(struct
device *dev)
  iommu_group_remove_device(dev);
  }
+static struct iommu_group *rk_iommu_device_group(struct device *dev)
+{
+struct iommu_group *group;
+int ret;
+
+group = iommu_group_get(dev);
+if (!group) {


This check is pointless - if dev->iommu_group were non-NULL you wouldn't
have been called in the first place.

right, it's allocated in the probe.


Robin.



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

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

[JeffyChen]

Hi Robin,

On 01/17/2018 08:18 PM, Robin Murphy wrote:


@@ -91,7 +92,6 @@ struct rk_iommu {
  void __iomem **bases;
  int num_mmu;
  int *irq;


Nit: irq seems to be redundant now as well.

oops, will fix it.



-int num_irq;
  bool reset_disabled;
  struct iommu_device iommu;
  struct list_head node; /* entry in rk_iommu_domain.iommus */
@@ -830,13 +830,6 @@ static int rk_iommu_attach_device(struct
iommu_domain *domain,
  iommu->domain = domain;
-for (i = 0; i < iommu->num_irq; i++) {
-ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq,
-   IRQF_SHARED, dev_name(dev), iommu);
-if (ret)
-return ret;
-}
-
  for (i = 0; i < iommu->num_mmu; i++) {
  rk_iommu_write(iommu->bases[i], RK_MMU_DTE_ADDR,
 rk_domain->dt_dma);
@@ -885,9 +878,6 @@ static void rk_iommu_detach_device(struct
iommu_domain *domain,
  }
  rk_iommu_disable_stall(iommu);
-for (i = 0; i < iommu->num_irq; i++)
-devm_free_irq(iommu->dev, iommu->irq[i], iommu);
-
  iommu->domain = NULL;
  dev_dbg(dev, "Detached from iommu domain\n");
@@ -1138,7 +1128,7 @@ static int rk_iommu_probe(struct platform_device
*pdev)
  struct rk_iommu *iommu;
  struct resource *res;
  int num_res = pdev->num_resources;
-int err, i;
+int err, i, irq, num_irq;
  iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
  if (!iommu)
@@ -1165,23 +1155,17 @@ static int rk_iommu_probe(struct
platform_device *pdev)
  if (iommu->num_mmu == 0)
  return PTR_ERR(iommu->bases[0]);
-iommu->num_irq = platform_irq_count(pdev);
-if (iommu->num_irq < 0)
-return iommu->num_irq;
-if (iommu->num_irq == 0)
-return -ENXIO;
-
-iommu->irq = devm_kcalloc(dev, iommu->num_irq, sizeof(*iommu->irq),
-  GFP_KERNEL);
-if (!iommu->irq)
-return -ENOMEM;
-
-for (i = 0; i < iommu->num_irq; i++) {
-iommu->irq[i] = platform_get_irq(pdev, i);
-if (iommu->irq[i] < 0) {
-dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]);
+num_irq = of_irq_count(dev->of_node);


To follow up on the other reply, I'm not sure you really need to count
the IRQs beforehand at all - you're going to be looping through
platform_get_irq() and handling errors anyway, so you may as well just
start at 0 and keep going until -ENOENT (or use platform_get_resource()
to double-check whether an index should be valid, as we do in arm_smmu).

ok, will do that.


Otherwise, it looks like everything that the IRQ handler needs in the
iommu struct (dev, num_mmu and bases) is already initialised by this
point, so we should be OK with respect to races.

ok.


Robin.



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

Re: [PATCH v2 05/13] iommu/rockchip: Fix error handling in init

[JeffyChen]

Hi Robin,

Thanks for your reply.

On 01/17/2018 07:36 PM, Robin Murphy wrote:

On 17/01/18 05:26, Tomasz Figa wrote:

On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen
 wrote:

It's hard to undo bus_set_iommu() in the error path, so move it to the
end of rk_iommu_probe().


Does this work fine now? I remember we used to need this called in an
early initcall for all the ARM/ARM64 DMA stuff to work.


It will do once we get to patch #11 (where the IOMMU_OF_DECLARE ensures
that masters defer until iommu_register() has set ops with a non-NULL
.of_xlate callback); in the meantime you might end up depending on DT
probe order as to whether the master uses the IOMMU or not. I'd say it's
up to you guys whether you consider that a bisection-breaker or not.


hmmm, maybe i can just place this patch after the patch #11 ;)

Robin.






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

Re: [PATCH v2 12/13] iommu/rockchip: Add runtime PM support

[JeffyChen]

Hi Tomasz,

On 01/17/2018 03:52 PM, JeffyChen wrote:

Hi Tomasz,

On 01/17/2018 03:38 PM, Tomasz Figa wrote:

>>Don't we need to check here (and in _shutdown() too) if we have a
>>domain attached?

>
>hmmm, right, the startup might been called by resume, so should check
>iommu->domain here.
>
>but the shutdown would be called at the end of detach or suspend, it
could
>be not attached or attached.

If startup might be called by resume, without domain attached, what
prevents shutdown from being called by suspend after that resume,
still without domain attached? Is it guaranteed that if resume is
called, someone will attach a domain before suspend is called?


no, the shutdown would be called by:
1/ end of detach_dev
so it would be not attached at that time

2/ suspend
so it could be attached, and also could be not attached


anyway, i think the shutdown would work without domain attached(just
disable paging and clear the iommu bases) ;)


hmmm, i see the problem.

so we need to:
1/ move shutdown a little earlier in detach_dev, so it could still see 
the iommu->domain


2/ check iommu->domain in shutdown, to prevent unnecessary shutdown


or maybe just add iommu->domain check in suspend and resume.

Best regards,
Tomasz








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

Re: [PATCH v2 12/13] iommu/rockchip: Add runtime PM support

[JeffyChen]

Hi Tomasz,

On 01/17/2018 03:38 PM, Tomasz Figa wrote:

>>Don't we need to check here (and in _shutdown() too) if we have a
>>domain attached?

>
>hmmm, right, the startup might been called by resume, so should check
>iommu->domain here.
>
>but the shutdown would be called at the end of detach or suspend, it could
>be not attached or attached.

If startup might be called by resume, without domain attached, what
prevents shutdown from being called by suspend after that resume,
still without domain attached? Is it guaranteed that if resume is
called, someone will attach a domain before suspend is called?


no, the shutdown would be called by:
1/ end of detach_dev
so it would be not attached at that time

2/ suspend
so it could be attached, and also could be not attached


anyway, i think the shutdown would work without domain attached(just 
disable paging and clear the iommu bases) ;)



Best regards,
Tomasz






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

Re: [PATCH v2 11/13] iommu/rockchip: Use OF_IOMMU to attach devices automatically

[JeffyChen]

On 01/17/2018 03:30 PM, Tomasz Figa wrote:

>but it's possible the probe failed after calling iommu_device_set_fwnode, so
>i'll try to add some checks here, and maybe adjust probe() to prevent it
>too.

I think iommu_device_set_fwnode() is not enough for of_iommu_xlate()
to find the IOMMU. The IOMMU is actually added to the IOMMU list by
iommu_device_register(), which is last in the sequence, so I guess we
should be fine.


hmmm, the later patch would change that ;) i'll fix it in the next version.


Best regards,
Tomasz






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

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

[JeffyChen]

Hi Tomasz,

On 01/17/2018 03:16 PM, Tomasz Figa wrote:

>>
>>This lacks consistency. of_irq_count() is used for counting, but
>>platform_get_irq() is used for getting. Either platform_ or of_ API
>>should be used for both and I'd lean for platform_, since it's more
>>general.

>
>hmmm, right, i was thinking of removing num_irq, and do something like:
>while (nr++) {
>   err = platform_get_irq(dev, nr);
>   if (err == -EPROBE_DEFER)
>  break;
>   if (err < 0)
>  return err;
>   
>}
>
>but forgot to do that..

Was there anything wrong with platform_irq_count() used by existing code?


just thought the platform_irq_count might ignore some errors(except for 
EPROBE_DEFER):


int platform_irq_count(struct platform_device *dev)
{
int ret, nr = 0;

while ((ret = platform_get_irq(dev, nr)) >= 0)
nr++;

if (ret == -EPROBE_DEFER)
return ret;

return nr;
}

but guess that would not matter..




>

>>

>>>+   if (irq < 0) {
>>>+   dev_err(dev, "Failed to get IRQ, %d\n", irq);
>>>  return -ENXIO;
>>>  }
>>>+   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
>>>+  IRQF_SHARED, dev_name(dev),
>>>iommu);
>>>+   if (err)
>>>+   return err;
>>>  }

>>
>>
>>Looks like there is some more initialization below. Is the driver okay
>>with the IRQ being potentially fired right here? (Shared IRQ handlers
>>might be run at request_irq() time for testing.)
>>

>right, forget about that. maybe we can check iommu->domain not NULL in
>rk_iommu_irq()

Maybe we could just move IRQ requesting to the end of probe?


ok, that should work too.

and maybe we should also check power status in the irq handler? if it 
get fired after powered off...



Best regards,
Tomasz






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

Re: [PATCH v2 12/13] iommu/rockchip: Add runtime PM support

[JeffyChen]

Hi Tomasz,

Thanks for your reply.

On 01/17/2018 02:20 PM, Tomasz Figa wrote:

On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen  wrote:

When the power domain is powered off, the IOMMU cannot be accessed and
register programming must be deferred until the power domain becomes
enabled.

Add runtime PM support, and use runtime PM device link from IOMMU to
master to startup and shutdown IOMMU.

[snip]

@@ -875,28 +889,19 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, 
unsigned long _iova,

  static struct rk_iommu *rk_iommu_from_dev(struct device *dev)
  {
-   return dev->archdata.iommu;
+   struct rk_iommudata *data = dev->archdata.iommu;
+
+   return data ? data->iommu : NULL;
  }


Is this change intentionally added to this patch? I see this
potentially relevant for the previous patch in this series.

right, will do that.



[snip]

+static int rk_iommu_startup(struct rk_iommu *iommu)
  {
-   struct rk_iommu *iommu;
+   struct iommu_domain *domain = iommu->domain;
 struct rk_iommu_domain *rk_domain = to_rk_domain(domain);
-   unsigned long flags;
 int ret, i;

-   /*
-* Allow 'virtual devices' (e.g., drm) to attach to domain.
-* Such a device does not belong to an iommu group.
-*/
-   iommu = rk_iommu_from_dev(dev);
-   if (!iommu)
-   return 0;
-
-   if (iommu->domain)
-   rk_iommu_detach_device(iommu->domain, dev);
-
 ret = rk_iommu_enable_clocks(iommu);
 if (ret)
 return ret;



Don't we need to check here (and in _shutdown() too) if we have a
domain attached?
hmmm, right, the startup might been called by resume, so should check 
iommu->domain here.


but the shutdown would be called at the end of detach or suspend, it 
could be not attached or attached.



+   mutex_lock(>pm_mutex);
 ret = rk_iommu_enable_stall(iommu);
 if (ret)
-   goto err_disable_clocks;
+   goto err_unlock_mutex;

[snip]

+   iommu->domain = NULL;
+
+   spin_lock_irqsave(_domain->iommus_lock, flags);
+   list_del_init(>node);
+   spin_unlock_irqrestore(_domain->iommus_lock, flags);
+
+   if (pm_runtime_get_if_in_use(iommu->dev) > 0) {


Actually, if the above call returns -EINVAL, don't we still need to
call rk_iommu_shutdown(), because it just means runtime PM is disabled
and the IOMMU is always powered on?

ok, will fix that.



+   rk_iommu_shutdown(iommu);
+   pm_runtime_put(iommu->dev);
+   }
+}

[snip]

+   spin_lock_irqsave(_domain->iommus_lock, flags);
+   list_add_tail(>node, _domain->iommus);
+   spin_unlock_irqrestore(_domain->iommus_lock, flags);
+
+   if (pm_runtime_get_if_in_use(iommu->dev) <= 0)


Ditto.


+   return 0;
+
+   ret = rk_iommu_startup(iommu);
+   if (ret)
+   rk_iommu_detach_device(data->domain, dev);

[snip]

@@ -1108,7 +1175,9 @@ static int rk_iommu_of_xlate(struct device *dev,
 return -ENODEV;
 }

-   dev->archdata.iommu = platform_get_drvdata(iommu_dev);
+   data->iommu = platform_get_drvdata(iommu_dev);
+   dev->archdata.iommu = data;
+


I think this change might be mistakenly squashed to this patch instead
of previous.

right, will do.


Best regards,
Tomasz






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

Re: [PATCH v2 11/13] iommu/rockchip: Use OF_IOMMU to attach devices automatically

[JeffyChen]

Hi Tomasz,

Thanks for your reply.

On 01/17/2018 01:44 PM, Tomasz Figa wrote:

On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen  wrote:

Converts the rockchip-iommu driver to use the OF_IOMMU infrastructure,
which allows attaching master devices to their IOMMUs automatically
according to DT properties.

Signed-off-by: Jeffy Chen 
---

Changes in v2: None

  drivers/iommu/rockchip-iommu.c | 116 +++--
  1 file changed, 31 insertions(+), 85 deletions(-)

diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 51e4f982c4a6..c2d3ac82184e 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c

[snip]

+static int rk_iommu_of_xlate(struct device *dev,
+struct of_phandle_args *args)
+{
+   struct platform_device *iommu_dev;
+
+   iommu_dev = of_find_device_by_node(args->np);
+   if (!iommu_dev) {
+   dev_err(dev, "iommu %pOF not found\n", args->np);
+   return -ENODEV;
+   }
+
+   dev->archdata.iommu = platform_get_drvdata(iommu_dev);


This will work only if that iommu was already probed. Do we have any
guarantees that this callback is not called earlier?
in of_iommu.c, the of_iommu_xlate would check for fwnode before calling 
this.


but it's possible the probe failed after calling 
iommu_device_set_fwnode, so i'll try to add some checks here, and maybe 
adjust probe() to prevent it too.




Best regards,
Tomasz






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

Re: [PATCH v2 02/13] iommu/rockchip: Suppress unbinding

[JeffyChen]

Hi Tomasz,

On 01/17/2018 01:32 PM, Tomasz Figa wrote:

On Wed, Jan 17, 2018 at 1:23 PM, Tomasz Figa  wrote:

On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen  wrote:

It's not safe to unbind rockchip IOMMU driver.


Might be good to explain why it is not safe and actually add that it
does not make any sense for such low level devices.


Actually, shouldn't we also remove support for .remove() and module
exit? I don't think it's actually feasible to unload this driver. We
actually even have ROCKCHIP_IOMMU Kconfig entry defined as bool, so
the driver can be only built-in.


right, will do it in the next version.

Best regards,
Tomasz






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

Re: [PATCH v2 05/13] iommu/rockchip: Fix error handling in init

[JeffyChen]

Hi Tomasz,

On 01/17/2018 01:26 PM, Tomasz Figa wrote:

On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen  wrote:

It's hard to undo bus_set_iommu() in the error path, so move it to the
end of rk_iommu_probe().


Does this work fine now? I remember we used to need this called in an
early initcall for all the ARM/ARM64 DMA stuff to work.

yes, i think it works now, i saw there are some other iommu drivers also 
do this(arm-smmu-v3, mtk_iommu) :)



Best regards,
Tomasz






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

Re: [PATCH v2 01/13] iommu/rockchip: Request irqs in rk_iommu_probe()

[JeffyChen]

Hi Tomasz,

Thanks for your reply.

On 01/17/2018 12:21 PM, Tomasz Figa wrote:

Hi Jeffy,

Thanks for the patch. Please see my comments inline.

On Tue, Jan 16, 2018 at 10:25 PM, Jeffy Chen  wrote:

Please add patch description.


ok, will do.



Suggested-by: Robin Murphy 
Signed-off-by: Jeffy Chen 
---

[snip]

-   for (i = 0; i < iommu->num_irq; i++) {
-   iommu->irq[i] = platform_get_irq(pdev, i);
-   if (iommu->irq[i] < 0) {
-   dev_err(dev, "Failed to get IRQ, %d\n", iommu->irq[i]);
+   num_irq = of_irq_count(dev->of_node);
+   for (i = 0; i < num_irq; i++) {
+   irq = platform_get_irq(pdev, i);


This lacks consistency. of_irq_count() is used for counting, but
platform_get_irq() is used for getting. Either platform_ or of_ API
should be used for both and I'd lean for platform_, since it's more
general.

hmmm, right, i was thinking of removing num_irq, and do something like:
while (nr++) {
  err = platform_get_irq(dev, nr);
  if (err == -EPROBE_DEFER)
 break;
  if (err < 0)
 return err;
  
}

but forgot to do that..




+   if (irq < 0) {
+   dev_err(dev, "Failed to get IRQ, %d\n", irq);
 return -ENXIO;
 }
+   err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
+  IRQF_SHARED, dev_name(dev), iommu);
+   if (err)
+   return err;
 }


Looks like there is some more initialization below. Is the driver okay
with the IRQ being potentially fired right here? (Shared IRQ handlers
might be run at request_irq() time for testing.)

right, forget about that. maybe we can check iommu->domain not NULL in 
rk_iommu_irq()




 iommu->reset_disabled = device_property_read_bool(dev,


^^

Best regards,
Tomasz






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

Re: [PATCH 1/9] iommu/of: Drop early initialisation hooks

[JeffyChen]

Hi Robin,

Thnaks for your reply.

On 01/11/2018 08:24 PM, Robin Murphy wrote:

Hi Jeffy,

On 11/01/18 11:14, JeffyChen wrote:

Hi Marek,

Thanks for your reply.

On 01/11/2018 05:40 PM, Marek Szyprowski wrote:

Hi Jeffy,

On 2018-01-11 09:22, Jeffy Chen wrote:

With the probe-deferral mechanism, early initialisation hooks are no
longer needed.

Suggested-by: Robin Murphy <robin.mur...@arm.com>


In fact, shortly after I said that I had a "how hard can it be?" moment
and took a crack at it myself - sorry, I should probably have cc'd you
on that series[1].


hmmm, i'll drop this patch in the next version.

and maybe rebase my patch[9] (iommu/rockchip: Use OF_IOMMU to attach 
devices automatically) on that series



Signed-off-by: Jeffy Chen <jeffy.c...@rock-chips.com>
---

  drivers/iommu/arm-smmu-v3.c  |  2 +-
  drivers/iommu/arm-smmu.c | 12 ++--
  drivers/iommu/exynos-iommu.c |  2 +-


For Exynos IOMMU:
Acked-by: Marek Szyprowski <m.szyprow...@samsung.com>

IPMMU and MSM IOMMU are no longer multi-platform safe after this patch.
It breaks them in the same way as my commit 928055a01b3f ("iommu/exynos:
Remove custom platform device registration code") broke Exynos IOMMU.

You need a similar fix for them:
https://www.spinics.net/lists/arm-kernel/msg627648.html


hmmm, right, i did saw this fix in the rockchip iommu driver too.

and there're also some other iommu drivers put bus_set_iommu in their
probe() to avoid that.

maybe we can do it in the iommu framework?

for example:
1/ add a bus type member to struct iommu_device
2/ and a iommu_device_set_bus()
3/ do the bus_set_iommu stuff in iommu_device_register()
4/ undo bus_set_iommu in iommu_device_unregister()


Ultimately we'd like to get rid of the bus relationship altogether, so I
don't think it's really worth adding more infrastructure around it.
Having of-iommu-based drivers set bus ops at probe time, and others
conditionally from an initcall, is pretty clean and simple, so I'd
rather stick with that approach for now.

ok, make sense:)


Robin.

[1]
https://lists.linuxfoundation.org/pipermail/iommu/2018-January/025395.html






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

Re: [PATCH 2/9] iommu/rockchip: Fix error handling in attach

[JeffyChen]

Hi Robin,

thanks for your reply.

On 01/11/2018 11:47 PM, Robin Murphy wrote:


+for (i = 0; i < iommu->num_irq; i++) {
+ret = devm_request_irq(iommu->dev, iommu->irq[i], rk_iommu_irq,
+   IRQF_SHARED, dev_name(dev), iommu);


Why aren't we simply requesting the IRQ once in rk_iommu_probe()? Given
that the hardware doesn't handle multiple translation contexts, there
doesn't seem to be much point in being this dynamic about it.


it make sense, will do it in next version:)

Robin.



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

Re: [PATCH 1/9] iommu/of: Drop early initialisation hooks

[JeffyChen]

Hi Marek,

Thanks for your reply.

On 01/11/2018 05:40 PM, Marek Szyprowski wrote:

Hi Jeffy,

On 2018-01-11 09:22, Jeffy Chen wrote:

With the probe-deferral mechanism, early initialisation hooks are no
longer needed.

Suggested-by: Robin Murphy 
Signed-off-by: Jeffy Chen 
---

  drivers/iommu/arm-smmu-v3.c  |  2 +-
  drivers/iommu/arm-smmu.c | 12 ++--
  drivers/iommu/exynos-iommu.c |  2 +-


For Exynos IOMMU:
Acked-by: Marek Szyprowski 

IPMMU and MSM IOMMU are no longer multi-platform safe after this patch.
It breaks them in the same way as my commit 928055a01b3f ("iommu/exynos:
Remove custom platform device registration code") broke Exynos IOMMU.

You need a similar fix for them:
https://www.spinics.net/lists/arm-kernel/msg627648.html


hmmm, right, i did saw this fix in the rockchip iommu driver too.

and there're also some other iommu drivers put bus_set_iommu in their 
probe() to avoid that.


maybe we can do it in the iommu framework?

for example:
1/ add a bus type member to struct iommu_device
2/ and a iommu_device_set_bus()
3/ do the bus_set_iommu stuff in iommu_device_register()
4/ undo bus_set_iommu in iommu_device_unregister()



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