[PATCH] arm64/dts: Add missing DMA Abort interrupt to Juno

[Robin Murphy]

The DMA-330 has an "irq_abort" interrupt line on which it signals faults
separately from the "irq[n:0]" channel interrupts. On Juno, this is
wired up to SPI 92; add it to the DT so that DMAC faults are correctly
reported for the driver to reset the thing, rather than leaving it
locked up and waiting to time out.

CC: Liviu Dudau <liviu.du...@arm.com>
CC: Sudeep Holla <sudeep.ho...@arm.com>
CC: Lorenzo Pieralisi <lorenzo.pieral...@arm.com>
Signed-off-by: Robin Murphy <robin.mur...@arm.com>
---
 arch/arm64/boot/dts/arm/juno-base.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi 
b/arch/arm64/boot/dts/arm/juno-base.dtsi
index dd5158e..e5b59ca 100644
--- a/arch/arm64/boot/dts/arm/juno-base.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
@@ -115,6 +115,7 @@
 ,
 ,
 ,
+,
 ,
 ,
 ,
-- 
1.9.1

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

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

[Robin Murphy]

On 18/12/15 08:09, Yong Wu wrote:

This patch adds support for mediatek m4u (MultiMedia Memory Management
Unit).

Signed-off-by: Yong Wu 
---
  drivers/iommu/Kconfig |  14 +
  drivers/iommu/Makefile|   1 +
  drivers/iommu/mtk_iommu.c | 734 ++
  3 files changed, 749 insertions(+)
  create mode 100644 drivers/iommu/mtk_iommu.c




diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
new file mode 100644
index 000..d000d31
--- /dev/null
+++ b/drivers/iommu/mtk_iommu.c


[...]


+#define REG_MMU_CTRL_REG   0x110
+#define F_MMU_PREFETCH_RT_REPLACE_MOD  BIT(4)
+#define F_MMU_TF_PROTECT_SEL(prot) (((prot) & 0x3) << 5)
+#define F_COHERENCE_EN BIT(8)


[...]


+static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
+{
+   u32 regval;
+   int ret;
+
+   ret = clk_prepare_enable(data->bclk);
+   if (ret) {
+   dev_err(data->dev, "Failed to enable iommu bclk(%d)\n", ret);
+   return ret;
+   }
+
+   regval = F_MMU_PREFETCH_RT_REPLACE_MOD |
+   F_MMU_TF_PROTECT_SEL(2) |
+   F_COHERENCE_EN;


I meant to ask this last time - does setting F_COHERENCE_EN here imply 
that the M4U is capable of cache-coherent page table walks, or something 
else? If it's the former, and assuming the MT8173 is actually wired up 
to support that, then you should add a dma-coherent property to its DT 
node in patch 5 (which will also save you all the cache flushes on page 
table updates).



+   writel_relaxed(regval, data->base + REG_MMU_CTRL_REG);
+
+   regval = F_L2_MULIT_HIT_EN |
+   F_TABLE_WALK_FAULT_INT_EN |
+   F_PREETCH_FIFO_OVERFLOW_INT_EN |
+   F_MISS_FIFO_OVERFLOW_INT_EN |
+   F_PREFETCH_FIFO_ERR_INT_EN |
+   F_MISS_FIFO_ERR_INT_EN;
+   writel_relaxed(regval, data->base + REG_MMU_INT_CONTROL0);
+
+   regval = F_INT_TRANSLATION_FAULT |
+   F_INT_MAIN_MULTI_HIT_FAULT |
+   F_INT_INVALID_PA_FAULT |
+   F_INT_ENTRY_REPLACEMENT_FAULT |
+   F_INT_TLB_MISS_FAULT |
+   F_INT_MISS_TRANSATION_FIFO_FAULT |
+   F_INT_PRETETCH_TRANSATION_FIFO_FAULT;
+   writel_relaxed(regval, data->base + REG_MMU_INT_MAIN_CONTROL);
+
+   regval = F_MMU_IVRP_PA_SET(data->protect_base);
+   writel_relaxed(regval, data->base + REG_MMU_IVRP_PADDR);
+
+   writel_relaxed(0, data->base + REG_MMU_DCM_DIS);
+   writel_relaxed(0, data->base + REG_MMU_STANDARD_AXI_MODE);
+
+   if (devm_request_irq(data->dev, data->irq, mtk_iommu_isr, 0,
+dev_name(data->dev), (void *)data)) {
+   writel_relaxed(0, data->base + REG_MMU_PT_BASE_ADDR);
+   clk_disable_unprepare(data->bclk);
+   dev_err(data->dev, "Failed @ IRQ-%d Request\n", data->irq);
+   return -ENODEV;
+   }
+
+   return 0;
+}


Otherwise, I've not had the chance to go through this thoroughly but at 
a glance it seems in pretty good shape now - nothing immediately jumps 
out as looking wrong or worth making a fuss over.


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

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

[Robin Murphy]

On 16/12/15 05:59, Yong Wu wrote:

On Tue, 2015-12-15 at 12:37 +, Robin Murphy wrote:

On 15/12/15 03:28, Yong Wu wrote:

On Mon, 2015-12-14 at 15:16 +0100, Joerg Roedel wrote:

On Tue, Dec 08, 2015 at 05:49:12PM +0800, Yong Wu wrote:

+static int mtk_iommu_attach_device(struct iommu_domain *domain,
+  struct device *dev)
+{
+   struct mtk_iommu_domain *dom = to_mtk_domain(domain);
+   struct mtk_iommu_client_priv *priv = dev->archdata.iommu;
+   struct mtk_iommu_data *data;
+   int ret;
+
+   if (!priv)
+   return -ENODEV;
+
+   data = dev_get_drvdata(priv->m4udev);
+   if (!data) {
+   /*
+* The DMA core will run earlier than this probe, and it will
+* create a default iommu domain for each a iommu device.
+* But here there is only one domain called the m4u domain
+* which all the multimedia HW share.
+* The default domain isn't needed here.
+*/


The iommu core creates one domain per iommu-group. In your case this
means one default domain per iommu in the system.


Yes. The iommu core will create one domain per iommu-group.
see the next "if" here.

But the domain here is created by the current DMA64. It's from this
function do_iommu_attach which will be called too early and will help
create a default domain for each a iommu device.(my codebase is
v4.4-rc1).


I still don't really understand the problem here. The M4U device is
created early in mtk_iommu_init_fn, so it should be probed and ready to
go long before anything else. Indeed, for your mtk_iommu_device_group()
callback to work then everything must already be happening in the right
order - add_device will only call iommu_group_get_for_dev() if of_xlate
has populated dev->archdata.iommu, and of_xlate will only do that if the
M4U was up and running before the client device started probing.
Furthermore, if mtk_iommu_device_group() *does* work, then the IOMMU
core will go ahead and allocate the default domain there and then, which
the arch code should find and use later.


Thanks. This is very helpful.

I understand your confuse right now and your expectant flow.

Our IOMMU probe was PROBE_DEFER by our SMI device, so currently it probe
was delayed, then have to add the workaround code.


Aha, I hadn't twigged that there was a dependency on another device that 
could delay the M4U probe, thanks for the clarification. That'll be a 
good case to bear in mind when I eventually get back to the IOMMU probe 
deferral stuff.



Following your comment above, I test as below. Then the flows seems meet
the "best case" that the iommu core will help create default DMA domain.

@@ -664,19 +636,41 @@ static int mtk_iommu_probe(struct platform_device
*pdev)
for (i = 0; i < larb_nr; i++) {
struct device_node *larbnode;
struct platform_device *plarbdev;

larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
if (!larbnode)
return -EINVAL;

 plarbdev = of_find_device_by_node(larbnode);
 of_node_put(larbnode);
-   if (!plarbdev)
-   return -EPROBE_DEFER;
+   if (!plarbdev) {
+   plarbdev = of_platform_device_create(larbnode,
NULL, platform_bus_type.dev_root);
+   if (IS_ERR(pdev))
+   return -EPROBE_DEFER;
+   }
}

I only add of_platform_device_create for the SMI local arbiter devices
here.

This is a big improvement for us. If this is ok, I will send a quick
next version for this.


In my opinion it's reasonable - we need the whole "IOMMU" to be ready, 
so if we already have to short-cut the creation of the M4U part it only 
seems fair to do the same for the SMI part. That said, would it work to 
just unconditionally poke the larbs in mtk_iommu_init_fn() before you 
poke the M4U itself? It would be nice to keep all that stuff together in 
the same place.



The potential issue I *do* see, looking more closely, is that
iommu_group_get_for_dev() is setting group->domain but not calling the
attach_dev callback, which looks wrong...


This is the backtrace,

(151216_09:58:05.207)Call trace:
(151216_09:58:05.207)[] mtk_iommu_attach_device
+0xb8/0x178
(151216_09:58:05.207)[] iommu_group_add_device
+0x1d8/0x31c
(151216_09:58:05.207)[] iommu_group_get_for_dev
+0x88/0x108
(151216_09:58:05.207)[] mtk_iommu_add_device+0x14/0x34
(151216_09:58:05.207)[] add_iommu_group+0x20/0x44
(151216_09:58:05.207)[] bus_for_each_dev+0x58/0x98
(151216_09:58:05.207)[] bus_set_iommu+0x9c/0xf8


Urgh, now I recall that this isn't even the first time I've been 
confused by the attach being hidden elsewhere. Oh well, problem averted!



If I change like above, I will delete the workaround code..

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

[Robin Murphy]

On 15/12/15 03:28, Yong Wu wrote:

On Mon, 2015-12-14 at 15:16 +0100, Joerg Roedel wrote:

On Tue, Dec 08, 2015 at 05:49:12PM +0800, Yong Wu wrote:

+static int mtk_iommu_attach_device(struct iommu_domain *domain,
+  struct device *dev)
+{
+   struct mtk_iommu_domain *dom = to_mtk_domain(domain);
+   struct mtk_iommu_client_priv *priv = dev->archdata.iommu;
+   struct mtk_iommu_data *data;
+   int ret;
+
+   if (!priv)
+   return -ENODEV;
+
+   data = dev_get_drvdata(priv->m4udev);
+   if (!data) {
+   /*
+* The DMA core will run earlier than this probe, and it will
+* create a default iommu domain for each a iommu device.
+* But here there is only one domain called the m4u domain
+* which all the multimedia HW share.
+* The default domain isn't needed here.
+*/


The iommu core creates one domain per iommu-group. In your case this
means one default domain per iommu in the system.


Yes. The iommu core will create one domain per iommu-group.
see the next "if" here.

But the domain here is created by the current DMA64. It's from this
function do_iommu_attach which will be called too early and will help
create a default domain for each a iommu device.(my codebase is
v4.4-rc1).


I still don't really understand the problem here. The M4U device is 
created early in mtk_iommu_init_fn, so it should be probed and ready to 
go long before anything else. Indeed, for your mtk_iommu_device_group() 
callback to work then everything must already be happening in the right 
order - add_device will only call iommu_group_get_for_dev() if of_xlate 
has populated dev->archdata.iommu, and of_xlate will only do that if the 
M4U was up and running before the client device started probing. 
Furthermore, if mtk_iommu_device_group() *does* work, then the IOMMU 
core will go ahead and allocate the default domain there and then, which 
the arch code should find and use later.


The potential issue I *do* see, looking more closely, is that 
iommu_group_get_for_dev() is setting group->domain but not calling the 
attach_dev callback, which looks wrong...




//=the next "if"===
} else if (!data->m4u_dom) {
/*
 * While a device is added into a iommu group, the iommu core
 * will create a default domain for each a iommu group.
 * This default domain is reserved as the m4u domain and is
 * initiated here.
 */
data->m4u_dom = dom;
if (domain->type == IOMMU_DOMAIN_DMA) {
ret = iommu_dma_init_domain(domain, 0,
DMA_BIT_MASK(32));
if (ret)
goto err_uninit_dom;
}

ret = mtk_iommu_domain_finalise(data);
if (ret)
goto err_uninit_dom;
}
//==




+   iommu_domain_free(domain);


This function is not supposed to free the domain passed to it.


As above this domain is created in the do_iommu_attach which will help
create a default domain for each a iommu device.
We don't need this default domain!

If we don't free it here, there will be a memory leak.

 From Robin's comment, He will improve the sequence of the
__iommu_setup_dma_ops in the future.


That already happened. The final version of the arm64 code which was 
merged makes sure that the IOMMU driver always sees the callbacks in the 
desired of_xlate -> add_device -> attach_dev order. The whole point of 
the comment below is that the driver itself *doesn't* have to care about 
the awkward way in which that is currently achieved.



/*
  * TODO: Right now __iommu_setup_dma_ops() gets called too early to do
  * everything it needs to - the device is only partially created and the
  * IOMMU driver hasn't seen it yet, so it can't have a group. Thus we
  * need this delayed attachment dance. Once IOMMU probe ordering is
sorted
  * to move the arch_setup_dma_ops() call later, all the notifier bits
below
  * become unnecessary, and will go away.
  */

/*
  * Best case: The device is either part of a group which was
  * already attached to a domain in a previous call, or it's
  * been put in a default DMA domain by the IOMMU core.
  */


That was before Joerg made the device_group changes which enabled proper 
default domains for platform devices - with those, we should be now be 
hitting the "best case" behaviour every time. In fact I think the "fake 
default domain" workaround shouldn't be needed at all any more, so I 
will add removing it to my giant list of things to do.



But there is no this patch currently, so I add iommu_domain_free
here.

"free the domain" here looks really not good. Then I delete the
iommu_domain_free here(allow this memory leak right now), is it ok?
(It will also works after Robin's change in the future.)




+static int 

Re: [PATCH v5 2/4] drm: Add support for ARM's HDLCD controller.

[Robin Murphy]

On 08/12/15 16:52, Liviu Dudau wrote:

On Tue, Dec 08, 2015 at 04:25:27PM +, Robin Murphy wrote:

Hi Liviu,

On 07/12/15 12:11, Liviu Dudau wrote:

The HDLCD controller is a display controller that supports resolutions
up to 4096x4096 pixels. It is present on various development boards
produced by ARM Ltd and emulated by the latest Fast Models from the
company.

Cc: David Airlie <airl...@linux.ie>
Cc: Robin Murphy <robin.mur...@arm.com>


I've given this a spin on my Juno, and the first thing of note is this:

hdlcd 7ff6.hdlcd: master bind failed: -517
hdlcd 7ff5.hdlcd: master bind failed: -517
scpi_protocol scpi: SCP Protocol 1.0 Firmware 1.9.0 version
[drm] found ARM HDLCD version r0p0
tda998x 0-0070: Falling back to first CRTC
usb 1-1: new high-speed USB device number 2 using ehci-platform
tda998x 0-0070: found TDA19988
hdlcd 7ff6.hdlcd: bound 0-0070 (ops tda998x_ops)
[drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[drm] No driver support for vblank timestamp query.
[ cut here ]
WARNING: at drivers/gpu/drm/drm_atomic_helper.c:682
Modules linked in:

CPU: 2 PID: 98 Comm: kworker/u12:3 Tainted: GW   4.4.0-rc2+ #846
Hardware name: ARM Juno development board (r1) (DT)
Workqueue: deferwq deferred_probe_work_func
task: fe007ecb3700 ti: fe09409c8000 task.ti: fe09409c8000
PC is at drm_atomic_helper_update_legacy_modeset_state+0x1e8/0x1f0
LR is at drm_atomic_helper_commit_modeset_disables+0x1a8/0x388
pc : [] lr : [] pstate: 2045
sp : fe09409cb560
x29: fe09409cb560 x28: fe0940bf2800
x27: fe094007 x26: 0001
x25: febe4b50 x24: fe7ae820
x23: febe4b50 x22: fe0940bd1000
x21: fe0940bd1000 x20: 
x19: fe0940968000 x18: fe0940c8091c
x17: 0007 x16: 0001
x15: fe0940c8016f x14: 003c
x13:  x12: 04c904b4
x11: 04b104c9 x10: 04b004b0
x9 : 06f4 x8 : 06a40654
x7 : 06f40640 x6 : fe0940966480
x5 : fe0940968200 x4 : 0001
x3 : fe0940966a80 x2 : 
x1 : fe0940bd0900 x0 : fe0940bd0960

---[ end trace bdb6af69b29bf7ea ]---
Call trace:
[]
drm_atomic_helper_update_legacy_modeset_state+0x1e8/0x1f0
[] drm_atomic_helper_commit_modeset_disables+0x1a8/0x388
[] drm_atomic_helper_commit+0xd8/0x140
[] hdlcd_atomic_commit+0x10/0x18
[] drm_atomic_commit+0x40/0x70
[] restore_fbdev_mode+0x270/0x2b0
[] drm_fb_helper_restore_fbdev_mode_unlocked+0x34/0x90
[] drm_fb_helper_set_par+0x2c/0x60
[] fbcon_init+0x4d0/0x520
[] visual_init+0xac/0x108
[] do_bind_con_driver+0x1d4/0x3e8
[] do_take_over_console+0x174/0x1e8
[] do_fbcon_takeover+0x74/0x100
[] fbcon_event_notify+0x77c/0x7d8
[] notifier_call_chain+0x50/0x90
[] __blocking_notifier_call_chain+0x4c/0x90
[] blocking_notifier_call_chain+0x14/0x20
[] fb_notifier_call_chain+0x1c/0x28
[] register_framebuffer+0x1c0/0x2b8
[] drm_fb_helper_initial_config+0x284/0x3e8
[] drm_fbdev_cma_init+0x94/0x148
[] hdlcd_drm_bind+0x1d4/0x418
[] try_to_bring_up_master.part.2+0xc8/0x110
[] component_add+0x90/0x108
[] tda998x_probe+0x18/0x20
[] i2c_device_probe+0x164/0x228
[] driver_probe_device+0x1ec/0x2f0
[] __device_attach_driver+0x90/0xd8
[] bus_for_each_drv+0x58/0x98
[] __device_attach+0xc4/0x148
[] device_initial_probe+0x10/0x18
[] bus_probe_device+0x94/0xa0
[] deferred_probe_work_func+0x70/0xa8
[] process_one_work+0x138/0x378
[] worker_thread+0x124/0x498
[] kthread+0xdc/0xf0
[] ret_from_fork+0x10/0x50
Console: switching to colour frame buffer device 150x100

which for reference, is the first one in that function:

...
/* clear out existing links and update dpms */
for_each_connector_in_state(old_state, connector, old_conn_state, i) {
if (connector->encoder) {
WARN_ON(!connector->encoder->crtc);
...

That's on 4.4-rc2 with this series plus the 3 tda998x patches from Russell's
patch system applied. Is there something else I'm missing or does this need
looking at (could it be related to that initial probe deferral)?


Yeah, you also need Thierry Reding's patch to not set the connector->encoder in
drivers.

http://lists.freedesktop.org/archives/dri-devel/2015-November/094576.html


Ah, right, I don't think that one was specifically called out anywhere, 
but it does indeed make the splat go away, thanks!





Signed-off-by: Liviu Dudau <liviu.du...@arm.com>
Acked-by: Daniel Vetter <daniel.vet...@ffwll.ch>
---
  drivers/gpu/drm/Kconfig  |   2 +
  drivers/gpu/drm/Makefile |   1 +
  drivers/gpu/drm/arm/Kconfig  |  29 ++
  drivers/gpu/drm/arm/Makefile |   2 +
  drivers/gpu/drm/arm/hdlcd_crtc.c | 329 ++
  drivers/gpu/drm/arm/hdlcd_drv.c  | 580 +++
  drivers/gpu/drm/arm/hdlcd_drv.h  |  42 +++
  drivers/gpu/drm/arm

Re: [PATCH v5 2/4] drm: Add support for ARM's HDLCD controller.

[Robin Murphy]

Hi Liviu,

On 07/12/15 12:11, Liviu Dudau wrote:

The HDLCD controller is a display controller that supports resolutions
up to 4096x4096 pixels. It is present on various development boards
produced by ARM Ltd and emulated by the latest Fast Models from the
company.

>
> Cc: David Airlie <airl...@linux.ie>
> Cc: Robin Murphy <robin.mur...@arm.com>

I've given this a spin on my Juno, and the first thing of note is this:

hdlcd 7ff6.hdlcd: master bind failed: -517
hdlcd 7ff5.hdlcd: master bind failed: -517
scpi_protocol scpi: SCP Protocol 1.0 Firmware 1.9.0 version
[drm] found ARM HDLCD version r0p0
tda998x 0-0070: Falling back to first CRTC
usb 1-1: new high-speed USB device number 2 using ehci-platform
tda998x 0-0070: found TDA19988
hdlcd 7ff6.hdlcd: bound 0-0070 (ops tda998x_ops)
[drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[drm] No driver support for vblank timestamp query.
[ cut here ]
WARNING: at drivers/gpu/drm/drm_atomic_helper.c:682
Modules linked in:

CPU: 2 PID: 98 Comm: kworker/u12:3 Tainted: GW   4.4.0-rc2+ #846
Hardware name: ARM Juno development board (r1) (DT)
Workqueue: deferwq deferred_probe_work_func
task: fe007ecb3700 ti: fe09409c8000 task.ti: fe09409c8000
PC is at drm_atomic_helper_update_legacy_modeset_state+0x1e8/0x1f0
LR is at drm_atomic_helper_commit_modeset_disables+0x1a8/0x388
pc : [] lr : [] pstate: 2045
sp : fe09409cb560
x29: fe09409cb560 x28: fe0940bf2800
x27: fe094007 x26: 0001
x25: febe4b50 x24: fe7ae820
x23: febe4b50 x22: fe0940bd1000
x21: fe0940bd1000 x20: 
x19: fe0940968000 x18: fe0940c8091c
x17: 0007 x16: 0001
x15: fe0940c8016f x14: 003c
x13:  x12: 04c904b4
x11: 04b104c9 x10: 04b004b0
x9 : 06f4 x8 : 06a40654
x7 : 06f40640 x6 : fe0940966480
x5 : fe0940968200 x4 : 0001
x3 : fe0940966a80 x2 : 
x1 : fe0940bd0900 x0 : fe0940bd0960

---[ end trace bdb6af69b29bf7ea ]---
Call trace:
[] 
drm_atomic_helper_update_legacy_modeset_state+0x1e8/0x1f0

[] drm_atomic_helper_commit_modeset_disables+0x1a8/0x388
[] drm_atomic_helper_commit+0xd8/0x140
[] hdlcd_atomic_commit+0x10/0x18
[] drm_atomic_commit+0x40/0x70
[] restore_fbdev_mode+0x270/0x2b0
[] drm_fb_helper_restore_fbdev_mode_unlocked+0x34/0x90
[] drm_fb_helper_set_par+0x2c/0x60
[] fbcon_init+0x4d0/0x520
[] visual_init+0xac/0x108
[] do_bind_con_driver+0x1d4/0x3e8
[] do_take_over_console+0x174/0x1e8
[] do_fbcon_takeover+0x74/0x100
[] fbcon_event_notify+0x77c/0x7d8
[] notifier_call_chain+0x50/0x90
[] __blocking_notifier_call_chain+0x4c/0x90
[] blocking_notifier_call_chain+0x14/0x20
[] fb_notifier_call_chain+0x1c/0x28
[] register_framebuffer+0x1c0/0x2b8
[] drm_fb_helper_initial_config+0x284/0x3e8
[] drm_fbdev_cma_init+0x94/0x148
[] hdlcd_drm_bind+0x1d4/0x418
[] try_to_bring_up_master.part.2+0xc8/0x110
[] component_add+0x90/0x108
[] tda998x_probe+0x18/0x20
[] i2c_device_probe+0x164/0x228
[] driver_probe_device+0x1ec/0x2f0
[] __device_attach_driver+0x90/0xd8
[] bus_for_each_drv+0x58/0x98
[] __device_attach+0xc4/0x148
[] device_initial_probe+0x10/0x18
[] bus_probe_device+0x94/0xa0
[] deferred_probe_work_func+0x70/0xa8
[] process_one_work+0x138/0x378
[] worker_thread+0x124/0x498
[] kthread+0xdc/0xf0
[] ret_from_fork+0x10/0x50
Console: switching to colour frame buffer device 150x100

which for reference, is the first one in that function:

...
/* clear out existing links and update dpms */
for_each_connector_in_state(old_state, connector, old_conn_state, i) {
if (connector->encoder) {
WARN_ON(!connector->encoder->crtc);
...

That's on 4.4-rc2 with this series plus the 3 tda998x patches from 
Russell's patch system applied. Is there something else I'm missing or 
does this need looking at (could it be related to that initial probe 
deferral)?



Signed-off-by: Liviu Dudau <liviu.du...@arm.com>
Acked-by: Daniel Vetter <daniel.vet...@ffwll.ch>
---
  drivers/gpu/drm/Kconfig  |   2 +
  drivers/gpu/drm/Makefile |   1 +
  drivers/gpu/drm/arm/Kconfig  |  29 ++
  drivers/gpu/drm/arm/Makefile |   2 +
  drivers/gpu/drm/arm/hdlcd_crtc.c | 329 ++
  drivers/gpu/drm/arm/hdlcd_drv.c  | 580 +++
  drivers/gpu/drm/arm/hdlcd_drv.h  |  42 +++
  drivers/gpu/drm/arm/hdlcd_regs.h |  87 ++
  8 files changed, 1072 insertions(+)
  create mode 100644 drivers/gpu/drm/arm/Kconfig
  create mode 100644 drivers/gpu/drm/arm/Makefile
  create mode 100644 drivers/gpu/drm/arm/hdlcd_crtc.c
  create mode 100644 drivers/gpu/drm/arm/hdlcd_drv.c
  create mode 100644 drivers/gpu/drm/arm/hdlcd_drv.h
  create mode 100644 drivers/gpu/drm/arm/hdlcd_regs.h


[.

Re: Preprocessor arithmetic in dtsi files (base + offset)

[Robin Murphy]

On 26/11/15 16:23, Geert Uytterhoeven wrote:
[...]

I guess this would work, too?

scu_container@2000 {
 compatible = "simple-bus";

 ranges = <0x0 0x2000 0x1>;
 #address-cells = <1>;
 #size-cells = <1>;

 scu: scu@0 {
 compatible = "arm,cortex-a9-scu";
 reg = <0x 0x100>;

 gic: interrupt-controller@1000 {
 compatible = "arm,cortex-a9-gic";
 reg = <0x1000 0x1000>, <0x0100 0x0100>;

 twd-timer@0600 {
 compatible = "arm,cortex-a9-twd-timer";
 reg = <0x0600 0x10>;
};

No more explicit arithmetic needed, just substitue "2000".


Which, funnily enough, ends up looking an awful lot like the definition 
in the hardware documentation[1] too ;)


Robin.

[1]:http://infocenter.arm.com/help/topic/com.arm.doc.ddi0407i/CACCJFCJ.html


Gr{oetje,eeting}s,

 Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 -- Linus Torvalds

___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



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

Re: [PATCH v5 4/6] memory: mediatek: Add SMI driver

[Robin Murphy]

On 09/10/15 03:23, Yong Wu wrote:
[...]

+static int mtk_smi_probe(struct platform_device *pdev)
+{
+   struct device *dev = >dev;
+   struct mtk_smi_data *smidata;
+   int ret;
+
+   if (!dev->pm_domain)
+   return -EPROBE_DEFER;
+
+   smidata = devm_kzalloc(dev, sizeof(*smidata), GFP_KERNEL);
+   if (!smidata)
+   return -ENOMEM;
+
+   smidata->clk_apb = devm_clk_get(dev, "apb");
+   if (IS_ERR(smidata->clk_apb))
+   return PTR_ERR(smidata->clk_apb);
+
+   smidata->clk_smi = devm_clk_get(dev, "smi");
+   if (IS_ERR(smidata->clk_smi))
+   return PTR_ERR(smidata->clk_smi);
+
+   pm_runtime_enable(dev);
+   dev_set_drvdata(dev, smidata);
+   return ret;


ret is used uninitialised here, but you might as well just "return 0;" 
and get rid of the variable entirely.



+}


Robin.

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

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

[Robin Murphy]

On 09/10/15 03:23, Yong Wu wrote:
[...]

+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 


Nit: ordering?


+#include 
+#include "io-pgtable.h"


[...]

+struct mtk_iommu_data {
+   void __iomem*base;
+   int irq;
+   struct device   *dev;
+   struct device   *larbdev[MTK_IOMMU_LARB_MAX_NR];
+   struct clk  *bclk;
+   phys_addr_t protect_base; /* protect memory base */
+   int larb_nr;/* local arbiter number */
+   struct mtk_iommu_suspend_regreg;
+};


I think I've finally got my head round the way this hardware works - 
each LARB can be configured to block or allow transactions from the 
client device behind each port, but they _don't_ otherwise pass any 
information downstream such that the M4U itself can identify individual 
transactions, right? If that is indeed the case, then Joerg is totally 
correct that all clients of one M4U should be in a single group, so you 
might as well keep a handy iommu_group pointer here. I'll refer back to 
that idea later...


[...]

+static void mtk_iommu_clear_intr(const struct mtk_iommu_data *data)
+{
+   u32 val;
+
+   val = readl_relaxed(data->base + REG_MMU_INT_CONTROL0);
+   val |= F_INT_L2_CLR_BIT;
+   writel_relaxed(val, data->base + REG_MMU_INT_CONTROL0);
+}


Do you anticipate any other callers of this? AFAICS these 3 lines could 
just be rolled into mtk_iommu_isr().



+static void mtk_iommu_tlb_flush_all(void *cookie)
+{
+   struct mtk_iommu_domain *domain = cookie;
+   void __iomem *base;
+
+   base = domain->data->base;
+   writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
+   writel_relaxed(F_ALL_INVLD, base + REG_MMU_INVALIDATE);
+   mb();/* Make sure flush all done */


If it's purely to make sure the write has completed, would wmb() be 
sufficient here?



+}
+
+static void mtk_iommu_tlb_add_flush(unsigned long iova, size_t size,
+   bool leaf, void *cookie)
+{
+   struct mtk_iommu_domain *domain = cookie;
+   void __iomem *base = domain->data->base;
+   unsigned int iova_start = iova, iova_end = iova + size - 1;


Nit: why not simply name the argument iova_start in the first place, or 
just use iova below?



+   writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, base + REG_MMU_INV_SEL);
+
+   writel_relaxed(iova_start, base + REG_MMU_INVLD_START_A);
+   writel_relaxed(iova_end, base + REG_MMU_INVLD_END_A);
+   writel_relaxed(F_MMU_INV_RANGE, base + REG_MMU_INVALIDATE);
+}
+
+static void mtk_iommu_tlb_sync(void *cookie)
+{
+   struct mtk_iommu_domain *domain = cookie;
+   void __iomem *base = domain->data->base;
+   int ret;
+   u32 tmp;
+
+   ret = readl_poll_timeout_atomic(base + REG_MMU_CPE_DONE, tmp,
+   tmp != 0, 10, 100);
+   if (ret) {
+   dev_warn(domain->data->dev,
+"Partial TLB flush timed out, falling back to full 
flush\n");
+   mtk_iommu_tlb_flush_all(cookie);
+   }
+   writel_relaxed(0, base + REG_MMU_CPE_DONE);


Do you still need this writeback in the ret==0 case when you've already 
read CPE_DONE as 0, or should this be inside the condition? (in which 
case you could also use an early return to lose the indent)



+}

[...]

+static int mtk_iommu_hw_init(const struct mtk_iommu_domain *mtkdom)
+{
+   struct mtk_iommu_data *data = mtkdom->data;
+   void __iomem *base = data->base;
+   u32 regval;
+   int ret;
+
+   ret = clk_prepare_enable(data->bclk);
+   if (ret) {
+   dev_err(data->dev, "Failed to enable iommu clk(%d)\n", ret);
+   return ret;
+   }


I'm not sure about the asymmetry here; the clock gets enabled when 
attaching clients to a domain, but not disabled until the IOMMU itself 
is torn down in mtk_iommu_remove() (i.e. never). It seems like either 
the clock should be enabled in mtk_iommu_probe(), or disabled in domain 
detach.



+   writel_relaxed(mtkdom->cfg.arm_short_cfg.ttbr[0],
+  base + REG_MMU_PT_BASE_ADDR);
+
+   regval = F_MMU_PREFETCH_RT_REPLACE_MOD |
+   F_MMU_TF_PROTECT_SEL(2) |
+   F_COHERENCE_EN;
+   writel_relaxed(regval, base + REG_MMU_CTRL_REG);
+
+   regval = F_L2_MULIT_HIT_EN |
+   F_TABLE_WALK_FAULT_INT_EN |
+   F_PREETCH_FIFO_OVERFLOW_INT_EN |
+   F_MISS_FIFO_OVERFLOW_INT_EN |
+   F_PREFETCH_FIFO_ERR_INT_EN |
+   F_MISS_FIFO_ERR_INT_EN;
+   writel_relaxed(regval, base + REG_MMU_INT_CONTROL0);
+
+   regval = F_INT_TRANSLATION_FAULT |
+   F_INT_MAIN_MULTI_HIT_FAULT |
+   F_INT_INVALID_PA_FAULT |
+

Re: [PATCH V2 2/3] Add iommu driver for hi6220 SoC platform

[Robin Murphy]

On 20/10/15 09:45, Chen Feng wrote:

iommu/hisilicon: Add hi6220-SoC smmu driver


A brief description of the smmu and what capabilities it provides 
wouldn't go amiss here.



Signed-off-by: Chen Feng 
Signed-off-by: Yu Dongbin 
---
  drivers/iommu/Kconfig|   8 +
  drivers/iommu/Makefile   |   1 +
  drivers/iommu/hi6220_iommu.c | 482 +++
  3 files changed, 491 insertions(+)
  create mode 100644 drivers/iommu/hi6220_iommu.c

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 4664c2a..9b41708 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -343,6 +343,14 @@ config SPAPR_TCE_IOMMU
  Enables bits of IOMMU API required by VFIO. The iommu_ops
  is not implemented as it is not necessary for VFIO.

+config HI6220_IOMMU
+   bool "Hi6220 IOMMU Support"
+   depends on ARM64
+   select IOMMU_API
+   select IOMMU_IOVA
+   help
+ Enable IOMMU Driver for hi6220 SoC.
+
  # ARM IOMMU support
  config ARM_SMMU
bool "ARM Ltd. System MMU (SMMU) Support"
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index c6dcc51..ee4dfef 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -23,3 +23,4 @@ obj-$(CONFIG_EXYNOS_IOMMU) += exynos-iommu.o
  obj-$(CONFIG_SHMOBILE_IOMMU) += shmobile-iommu.o
  obj-$(CONFIG_SHMOBILE_IPMMU) += shmobile-ipmmu.o
  obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
+obj-$(CONFIG_HI6220_IOMMU) += hi6220_iommu.o


It would be nice to insert this before CONFIG_INTEL_IOMMU to try to 
maintain some semblance of order.



diff --git a/drivers/iommu/hi6220_iommu.c b/drivers/iommu/hi6220_iommu.c
new file mode 100644
index 000..6c5198d
--- /dev/null
+++ b/drivers/iommu/hi6220_iommu.c
@@ -0,0 +1,482 @@
+/*
+ * Hisilicon Hi6220 IOMMU driver
+ *
+ * Copyright (c) 2015 Hisilicon Limited.
+ *
+ * Author: Chen Feng 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#define pr_fmt(fmt) "IOMMU: " fmt
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 


Those last 3 spoil the ordering here. Plus, do you really need 
linux/interrupt.h twice?



+#define SMMU_CTRL_OFFSET (0x)
+#define SMMU_ENABLE_OFFSET   (0x0004)
+#define SMMU_PTBR_OFFSET (0x0008)
+#define SMMU_START_OFFSET(0x000C)
+#define SMMU_END_OFFSET  (0x0010)
+#define SMMU_INTMASK_OFFSET  (0x0014)
+#define SMMU_RINTSTS_OFFSET  (0x0018)
+#define SMMU_MINTSTS_OFFSET  (0x001C)
+#define SMMU_INTCLR_OFFSET   (0x0020)
+#define SMMU_STATUS_OFFSET   (0x0024)
+#define SMMU_AXIID_OFFSET(0x0028)
+#define SMMU_CNTCTRL_OFFSET  (0x002C)
+#define SMMU_TRANSCNT_OFFSET (0x0030)
+#define SMMU_L0TLBHITCNT_OFFSET  (0x0034)
+#define SMMU_L1TLBHITCNT_OFFSET  (0x0038)
+#define SMMU_WRAPCNT_OFFSET  (0x003C)
+#define SMMU_SEC_START_OFFSET(0x0040)
+#define SMMU_SEC_END_OFFSET  (0x0044)
+#define SMMU_VERSION_OFFSET  (0x0048)
+#define SMMU_IPTSRC_OFFSET   (0x004C)
+#define SMMU_IPTPA_OFFSET(0x0050)
+#define SMMU_TRBA_OFFSET (0x0054)
+#define SMMU_BYS_START_OFFSET(0x0058)
+#define SMMU_BYS_END_OFFSET  (0x005C)
+#define SMMU_RAM_OFFSET  (0x1000)
+#define SMMU_REGS_MAX(15)


This is confusing; I count 24 registers listed above, what is 15 the 
maximum of?



+#define SMMU_REGS_SGMT_END   (0x60)
+#define SMMU_CHIP_ID_V100(1)
+#define SMMU_CHIP_ID_V200(2)
+
+#define SMMU_REGS_OPS_SEGMT_START(0xf00)
+#define SMMU_REGS_OPS_SEGMT_NUMB (8)
+#define SMMU_REGS_AXI_SEGMT_START(0xf80)
+#define SMMU_REGS_AXI_SEGMT_NUMB (8)
+
+#define SMMU_INIT(0x1)
+#define SMMU_RUNNING (0x2)
+#define SMMU_SUSPEND (0x3)
+#define SMMU_STOP(0x4)
+#define SMMU_CTRL_INVALID(BIT(10))
+#define PAGE_ENTRY_VALID (0x1)
+
+#define IOVA_START_PFN   (1)
+#define IOPAGE_SHIFT (12)
+#define IOVA_PFN(addr)   ((addr) >> IOPAGE_SHIFT)
+#define IOVA_PAGE_SZ (SZ_4K)
+#define IOVA_START   (0x2000)


This doesn't match up - surely either IOVA_START_PFN should be 2, or 
IOVA_START should be 0x1000.



+#define IOVA_END (0x8000)
+
+struct hi6220_smmu {
+   unsigned int irq;
+   irq_handler_t smmu_isr;
+   void __iomem *reg_base;
+   struct clk *smmu_peri_clk;
+   struct clk *smmu_clk;
+   struct clk *media_sc_clk;
+ 

Re: [PATCH v4 3/6] iommu: add ARM short descriptor page table allocator.

[Robin Murphy]

On 09/10/15 16:57, Will Deacon wrote:

On Tue, Sep 22, 2015 at 03:12:47PM +0100, Yong Wu wrote:

  I would like to show you a problem I met, The recursion here may
lead to stack overflow while we test FHD video decode.

 From the log, I get the internal variable in the error case: the
"size" is 0x10, the "iova" is 0xfea0, but at that time the
"blk_size" is 0x1000 as it was the map of small-page. so it enter the
recursion here.

 After check the unmap flow, there is only a iommu_unmap in
__iommu_dma_unmap, and it won't check the physical address align in
iommu_unmap.


That sounds like a bug in __iommu_dma_unmap. Robin?


Isn't it just cf27ec930be9 again wearing different trousers? All I do is 
call iommu_unmap with the same total size that was mapped originally.


Robin.

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

Re: [PATCH v2 3/3] irqchip/gicv3-its: Handle OF device tree "msi-map" properties.

[Robin Murphy]

On 23/09/15 19:18, Marc Zyngier wrote:

On Wed, 23 Sep 2015 18:52:59 +0100
Will Deacon  wrote:


On Wed, Sep 23, 2015 at 06:08:39PM +0100, David Daney wrote:

On 09/23/2015 10:01 AM, Marc Zyngier wrote:

On Tue, 22 Sep 2015 17:00:06 -0700
David Daney  wrote:


From: David Daney 

Call of_msi_map_rid() to handle mapping of the requester id.

Signed-off-by: David Daney 
---
   drivers/irqchip/irq-gic-v3-its-pci-msi.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c 
b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
index cf351c6..8b1c938 100644
--- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
@@ -86,7 +86,8 @@ static int its_pci_msi_prepare(struct irq_domain *domain, 
struct device *dev,
pci_for_each_dma_alias(pdev, its_get_pci_alias, _alias);

/* ITS specific DeviceID, as the core ITS ignores dev. */
-   info->scratchpad[0].ul = dev_alias.dev_id;
+   info->scratchpad[0].ul = of_msi_map_rid(dev, domain->of_node,
+   dev_alias.dev_id);

return msi_info->ops->msi_prepare(domain->parent,
  dev, dev_alias.count, info);


I really wonder if that shouldn't be part of the pci_for_each_dma_alias
call. It would make a lot more sense for this functionality to be an
integral part of the core code, and would probably make the integration
of _IORT (which has the exact same requirements) a bit easier.

Thoughts?



I am a proponent of pushing things like this as far into the core code
as possible.  So, from that point of view, I think it would probably be
a good idea.

I can prepare a patch that does that, but it would also be nice hear
from other maintainers and get their thoughts on this.


Hmm, we use pci_for_each_dma_alias in the SMMU drivers to get the SID,
so I'm not sure that using the MSI mapping is necessarily the right thing
to do there. Maybe we should instead have dma_alias_to_msi_id helpers or
something?


Yes, that's probably a sensible solution.


Seconded; take a look at all the additional bus-walking 
iommu_group_get_for_pci_dev does between the call to 
pci_for_each_dma_alias and the group = iommu_group_alloc() line - I'd 
say it's only at the latter point, when there are no aliases found on 
the PCI side, that it would then need to check the external RID-SID 
mapping to see if there is any further aliasing downstream of the root 
complex (and possibly liaise with the IOMMU driver to retrieve an 
appropriate group, but let's worry about that bit later).


Robin.



M.



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

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

[Robin Murphy]

On 03/08/15 11:21, Yong Wu wrote:

This patch adds support for mediatek m4u (MultiMedia Memory Management
Unit).

Signed-off-by: Yong Wu 
---

[...]

+/*
+ * There is only one iommu domain called the m4u domain that
+ * all Multimedia modules share.
+ */
+static struct mtk_iommu_domain *m4udom;


It's a shame this can't be part of the m4u device's mtk_iommu_data, but 
since the way iommu_domain_alloc works makes that impossible, I think we 
have little choice but to use the global and hope your guys never build 
a system with two of these things in ;)


[...]

+static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
+{
+   struct mtk_iommu_domain *priv;
+
+   if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA)
+   return NULL;
+
+   if (m4udom)/* The m4u domain exist. */
+   return >domain;
+
+   priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+   if (!priv)
+   return NULL;
+
+   priv->domain.geometry.aperture_start = 0;
+   priv->domain.geometry.aperture_end = DMA_BIT_MASK(32);
+   priv->domain.geometry.force_aperture = true;


My intention is that in the IOMMU_DOMAIN_DMA case you'd call 
iommu_get_dma_cookie(>domain) here as well, that way I can get rid 
of some of the dodgy workarounds in arch_setup_dma_ops which try to 
cover all possible cases (and which I'm now not 100% confident in). I'm 
just about to start trying to fix that up (expect a repost of my series 
in a week or two once -rc1 has landed).



+
+   m4udom = priv;
+
+   return >domain;
+}

[...]

+static int mtk_iommu_add_device(struct device *dev)
+{
+   struct iommu_group *group;
+   int ret;
+
+   if (!dev->archdata.iommu) /* Not a iommu client device */
+   return -ENODEV;
+
+   group = iommu_group_get(dev);
+   if (!group) {
+   group = iommu_group_alloc();
+   if (IS_ERR(group)) {
+   dev_err(dev, "Failed to allocate IOMMU group\n");
+   return PTR_ERR(group);
+   }
+   }
+

> +   ret = iommu_group_add_device(group, dev);

+   if (ret) {
+   dev_err(dev, "Failed to add IOMMU group\n");
+   goto err_group_put;
+   }


I know the rest of the code means that you can't hit it in practice, but 
if you ever did have two client devices in the same group then the 
iommu_group_get() could legitimately succeed for the second device, then 
you'd blow up creating a duplicate sysfs entry by adding the device to 
its own group again. Probably not what you want.



+
+   ret = iommu_attach_group(>domain, group);
+   if (ret)
+   dev_err(dev, "Failed to attach IOMMU group\n");


Similarly here, if two devices did share a group then the group could 
legitimately already be attached to a domain here (by the first device), 
so attaching it again would be wrong. I think it would be nicer to check 
with iommu_get_domain_for_dev() first to see if you need to do anything 
at all (a valid domain from that implies a valid group).



+
+err_group_put:
+   iommu_group_put(group);
+   return ret;
+}

[...]

+static int mtk_iommu_probe(struct platform_device *pdev)
+{
+   struct mtk_iommu_data   *data;
+   struct device   *dev = >dev;
+   void __iomem*protect;
+   int ret;
+
+   data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+   if (!data)
+   return -ENOMEM;
+
+   /* Protect memory. HW will access here while translation fault.*/
+   protect = devm_kzalloc(dev, MTK_PROTECT_PA_ALIGN * 2, GFP_KERNEL);
+   if (!protect)
+   return -ENOMEM;
+   data->protect_base = virt_to_phys(protect);
+
+   ret = mtk_iommu_parse_dt(pdev, data);
+   if (ret)
+   return ret;
+
+   if (!m4udom)/* There is no iommu client */
+   return 0;


I don't quite follow this: m4udom is apparently only created by someone 
calling domain_alloc() - how can you guarantee that happens before this 
driver is probed? - but if they then go and try to attach the device to 
their new domain, it's going to end up in mtk_hw_init() poking the 
hardware of the m4u device that can't have even probed yet.


I can only imagine it currently works by sheer chance due to the 
horrible arch_setup_dma_ops delayed attachment workaround, so even if I 
can't remove that completely when I look at it next week I'm liable to 
change it in a way that breaks this badly ;)


Robin.


+
+   data->dev = dev;
+   m4udom->data = data;
+   dev_set_drvdata(dev, m4udom);
+
+   return 0;
+}


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

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

[Robin Murphy]

On 16/07/15 10:04, Yong Wu wrote:

This patch adds support for mediatek m4u (MultiMedia Memory Management
Unit).

Signed-off-by: Yong Wu yong...@mediatek.com

[...]

+static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
+{
+   struct mtk_iommu_domain *domain = cookie;
+   unsigned long offset = (unsigned long)ptr  ~PAGE_MASK;
+
+   dma_map_page(domain-data-dev, virt_to_page(ptr), offset,
+size, DMA_TO_DEVICE);


Nit: this looks like it may as well be dma_map_single.

It would probably be worth following it with a matching unmap too, just 
to avoid any possible leakage bugs (especially if this M4U ever appears 
in a SoC supporting RAM above the 32-bit boundary).


 +}
 +
[...]

+static int mtk_iommu_parse_dt(struct platform_device *pdev,
+ struct mtk_iommu_data *data)
+{
+   struct device *dev = pdev-dev;
+   struct device_node *ofnode;
+   struct resource *res;
+   int i;
+
+   ofnode = dev-of_node;
+
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   data-base = devm_ioremap_resource(pdev-dev, res);
+   if (IS_ERR(data-base))
+   return PTR_ERR(data-base);
+
+   data-irq = platform_get_irq(pdev, 0);
+   if (data-irq  0)
+   return data-irq;
+
+   data-bclk = devm_clk_get(dev, bclk);
+   if (IS_ERR(data-bclk))
+   return PTR_ERR(data-bclk);
+
+   data-larb_nr = of_count_phandle_with_args(
+   ofnode, mediatek,larb, NULL);
+   if (data-larb_nr  0)
+   return data-larb_nr;
+
+   for (i = 0; i  data-larb_nr; i++) {
+   struct device_node *larbnode;
+   struct platform_device *plarbdev;
+
+   larbnode = of_parse_phandle(ofnode, mediatek,larb, i);
+   if (!larbnode)
+   return -EINVAL;
+
+   plarbdev = of_find_device_by_node(larbnode);
+   of_node_put(larbnode);
+   if (!plarbdev)
+   return -EPROBE_DEFER;
+   data-larbdev[i] = plarbdev-dev;
+   }


At a glance this seems like a job for of_parse_phandle_with_args, but I 
may be missing something subtle.



+   return 0;
+}
+

[...]

+static int mtk_iommu_init_domain_context(struct mtk_iommu_domain *dom)
+{
+   int ret;
+
+   if (dom-iop)
+   return 0;
+
+   spin_lock_init(dom-pgtlock);
+   dom-cfg.quirks = IO_PGTABLE_QUIRK_ARM_NS |
+   IO_PGTABLE_QUIRK_SHORT_SUPERSECTION |
+   IO_PGTABLE_QUIRK_SHORT_MTK;
+   dom-cfg.pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap,
+   dom-cfg.ias = 32;
+   dom-cfg.oas = 32;
+   dom-cfg.tlb = mtk_iommu_gather_ops;
+
+   dom-iop = alloc_io_pgtable_ops(ARM_SHORT_DESC, dom-cfg, dom);
+   if (!dom-iop) {
+   pr_err(Failed to alloc io pgtable\n);
+   return -EINVAL;
+   }
+
+   /* Update our support page sizes bitmap */
+   mtk_iommu_ops.pgsize_bitmap = dom-cfg.pgsize_bitmap;
+
+   ret = mtk_iommu_hw_init(dom);
+   if (ret)
+   free_io_pgtable_ops(dom-iop);
+
+   return ret;
+}


I don't see that you need the above function at all - since your 
pagetable config is fixed and doesn't have any depency on which IOMMU 
you're attaching to, can't you just do all of that straight away in 
domain_alloc?



+static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type)
+{
+   struct mtk_iommu_domain *priv;
+
+   /* We only support unmanaged domains for now */
+   if (type != IOMMU_DOMAIN_UNMANAGED)
+   return NULL;


Have you had a chance to play with the latest DMA mapping series now 
that I've integrated the default domain changes? I think if you handled 
IOMMU_DOMAIN_DMA requests here and made them always return the 
(preallocated) private domain, you should be able to get rid of the 
tricks in attach_device completely.



+   priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+   if (!priv)
+   return NULL;
+
+   priv-domain.geometry.aperture_start = 0;
+   priv-domain.geometry.aperture_end = (1ULL  32) - 1;


DMA_BIT_MASK(32) ?


+   priv-domain.geometry.force_aperture = true;
+
+   return priv-domain;
+}
+

[...]

+static int mtk_iommu_add_device(struct device *dev)
+{
+   struct mtk_iommu_domain *mtkdom;
+   struct iommu_group *group;
+   struct mtk_iommu_client_priv *devhead;
+   int ret;
+
+   if (!dev-archdata.dma_ops)/* Not a iommu client device */
+   return -ENODEV;


I think archdata.dma_ops is the wrong thing to test here. It would be 
better to use archdata.iommu, since you go on to dereference that 
unconditionally anyway, plus then you only depend on your own of_xlate 
behaviour, rather than some quirk of the arch code (which could quietly 
change in future).



+   group = iommu_group_get(dev);
+   if (!group) 

Re: [PATCH v3 3/6] iommu: add ARM short descriptor page table allocator.

[Robin Murphy]

On 27/07/15 05:21, Yong Wu wrote:
[...]

+static arm_short_iopte
+__arm_short_pte_prot(struct arm_short_io_pgtable *data, int prot, bool large)
+{
+   arm_short_iopte pteprot;
+
+   pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
+   pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
+   ARM_SHORT_PTE_TYPE_SMALL;
+   if (prot  IOMMU_CACHE)
+   pteprot |=  ARM_SHORT_PTE_B | ARM_SHORT_PTE_C;
+   if (prot  IOMMU_WRITE)
+   pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
+   ARM_SHORT_PTE_SMALL_TEX0;


This doesn't make any sense. TEX[2:0] is all about memory attributes, not
permissions, so you're making the mapping write-back, write-allocate but
that's not what the IOMMU_* values are about.


  I will delete it.


Well, can you not control mapping permissions with the AP bits? The idea
of the IOMMU flags are:

   IOMMU_CACHE : Install a normal, cacheable mapping (you've got this right)
   IOMMU_READ : Allow read access for the device
   IOMMU_WRITE : Allow write access for the device
   IOMMU_NOEXEC : Disallow execute access for the device

so the caller to iommu_map passes in a bitmap of these, which you need to
encode in the page-table entry.


 From the spec, AP[2] differentiate the read/write and readonly.
How about this?:
//===
   #define ARM_SHORT_PGD_FULL_ACCESS  (3  10)
   #define ARM_SHORT_PGD_RDONLY   BIT(15)

   pgdprot |= ARM_SHORT_PGD_FULL_ACCESS;/* or other names? */
   if(!(prot  IOMMU_WRITE)  (prot  IOMMU_READ))
  pgdprot |= ARM_SHORT_PGD_RDONLY;
//===
pte is the same.

Sorry, Our HW don't meet the standard spec fully. it don't implement the
AP bits.




+static int
+_arm_short_map(struct arm_short_io_pgtable *data,
+  unsigned int iova, phys_addr_t paddr,
+  arm_short_iopte pgdprot, arm_short_iopte pteprot,
+  bool large)
+{
+   const struct iommu_gather_ops *tlb = data-iop.cfg.tlb;
+   arm_short_iopte *pgd = data-pgd, *pte;
+   void *cookie = data-iop.cookie, *pte_va;
+   unsigned int ptenr = large ? 16 : 1;
+   int i, quirk = data-iop.cfg.quirks;
+   bool ptenew = false;
+
+   pgd += ARM_SHORT_PGD_IDX(iova);
+
+   if (!pteprot) { /* section or supersection */
+   if (quirk  IO_PGTABLE_QUIRK_SHORT_MTK)
+   pgdprot = ~ARM_SHORT_PGD_SECTION_XN;
+   pte = pgd;
+   pteprot = pgdprot;
+   } else {/* page or largepage */
+   if (quirk  IO_PGTABLE_QUIRK_SHORT_MTK) {
+   if (large) { /* special Bit */


This definitely needs a better comment! What exactly are you doing here
and what is that quirk all about?


I use this quirk is for MTK Special Bit as we don't have the XN bit in
pagetable.


I'm still not really clear about what this is.


There is some difference between the standard spec and MTK HW,
Our hw don't implement some bits, like XN and AP.
So I add a quirk for MTK special.


When you say it doesn't implement these bits, do you mean that having 
them set will lead to Bad Things happening in the hardware, or that it 
will simply ignore them and not enforce any of the protections they 
imply? The former case would definitely want clearly documenting 
somewhere, whereas for the latter case I'm not sure it's even worth the 
complication of having a quirk - if the value doesn't matter there seems 
little point in doing a special dance just for the sake of semantic 
correctness of the in-memory PTEs, in my opinion.


Robin.

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

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

[Robin Murphy]

On 27/07/15 16:31, Russell King - ARM Linux wrote:

On Mon, Jul 27, 2015 at 02:23:26PM +0100, Robin Murphy wrote:

On 16/07/15 10:04, Yong Wu wrote:

This patch adds support for mediatek m4u (MultiMedia Memory Management
Unit).

Signed-off-by: Yong Wu yong...@mediatek.com

[...]

+static void mtk_iommu_flush_pgtable(void *ptr, size_t size, void *cookie)
+{
+   struct mtk_iommu_domain *domain = cookie;
+   unsigned long offset = (unsigned long)ptr  ~PAGE_MASK;
+
+   dma_map_page(domain-data-dev, virt_to_page(ptr), offset,
+size, DMA_TO_DEVICE);


Nit: this looks like it may as well be dma_map_single.

It would probably be worth following it with a matching unmap too, just to
avoid any possible leakage bugs (especially if this M4U ever appears in a
SoC supporting RAM above the 32-bit boundary).


Why not do the job properly?  Take a look at how I implemented the
streaming DMA API on Tegra SMMU (patch set recently sent out earlier
today).

There's no need for hacks like dma_map_page() (and discarding it's
return value) or dma_map_page() followed by dma_unmap_page().


Indeed, as it happens I do have a branch where I prototyped that for the 
long-descriptor io-pgtable-arm code a while ago; this discussion has 
prompted me to dig it up again. Stay tuned, folks...


Robin.

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

Re: [PATCH 3/3] Docs: dt: add PCI IOMMU map bindings

[Robin Murphy]

Hi Mark,

This looks sane, and lets me describe the thing I have on my desk, so 
I'm happy. I have a couple of general thoughts below, but I don't intend 
that they should stand in the way of this proposal as-is.


On 23/07/15 17:52, Mark Rutland wrote:

The existing IOMMU bindings are able to specify the relationship between
masters and IOMMUs, but they are insufficient for describing the general
case of hotpluggable busses such as PCI where the set of masters is not
known until runtime, and the relationship between masters and IOMMUs is
a property of the integration of the system.

This patch adds a generic binding for mapping PCI devices to IOMMUs,
using a new iommu-map property (specific to PCI*) which may be used to
map devices (identified by their Requester ID) to sideband data for the
IOMMU which they master through.

Signed-off-by: Mark Rutland mark.rutl...@arm.com
---
  .../devicetree/bindings/pci/pci-iommu.txt  | 171 +
  1 file changed, 171 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/pci/pci-iommu.txt

diff --git a/Documentation/devicetree/bindings/pci/pci-iommu.txt 
b/Documentation/devicetree/bindings/pci/pci-iommu.txt
new file mode 100644
index 000..56c8296
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/pci-iommu.txt
@@ -0,0 +1,171 @@
+This document describes the generic device tree binding for describing the
+relationship between PCI(e) devices and IOMMU(s).
+
+Each PCI(e) device under a root complex is uniquely identified by its Requester
+ID (AKA RID). A Requester ID is a triplet of a Bus number, Device number, and
+Function number.
+
+For the purpose of this document, when treated as a numeric value, a RID is
+formatted such that:
+
+* Bits [15:8] are the Bus number.
+* Bits [7:3] are the Device number.
+* Bits [2:0] are the Function number.
+* Any other bits required for padding must be zero.
+
+IOMMUs may distinguish PCI devices through sideband data derived from the
+Requester ID. While a given PCI device can only master through one IOMMU, a
+root complex may split masters across a set of IOMMUs (e.g. with one IOMMU per
+bus).
+
+The generic 'iommus' property is insufficient to describe this relationship,
+and a mechanism is required to map from a PCI device to its IOMMU and sideband
+data.
+
+For generic IOMMU bindings, see
+Documentation/devicetree/bindings/iommu/iommu.txt.
+
+
+PCI root complex
+
+
+Optional properties
+---
+
+- iommu-map: Maps a Requester ID to an IOMMU and associated iommu-specifier
+  data.
+
+  The property is an arbitrary number of tuples of
+  (rid-base,iommu,iommu-base,length).
+
+  Any RID r in the interval [rid-base, rid-base + length) is associated with
+  the listed IOMMU, with the iommu-specifier (r - rid-base + iommu-base).


Can we take as a guarantee that the system cannot present any ID at a 
given IOMMU that is not represented in an appropriate output range (in 
the sense that we may do things that could blow up horribly if spurious 
IDs appeared)?


Furthermore, would representing one-to-many mappings by having multiple 
matches for a given RID be legal? In the general case it's certainly 
feasible for the IOMMU to see different IDs for e.g. reads vs. writes, 
where the system munges extra bus lines into the sideband signals - 
whether anyone would actually integrate a PCI host controller that way 
is another matter, so I don't think it's something worth really worrying 
about without a definite need.



+
+- iommu-map-mask: A mask to be applied to each Requester ID prior to being
+  mapped to an iommu-specifier per the iommu-map property.


Am I right to assume a mask of 0 would be a valid way to represent 
everything (and if so, should rid-base and length just be ignored, or 
mandated to be 0 and 1 respectively)? It looks a bit off at first 
glance, but it does neatly address a genuine use-case.



+
+
+Example (1)
+===
+
+/ {
+   #address-cells = 1;
+   #size-cells = 1;
+
+   iommu: iommu@a {
+   reg = 0xa 0x1;
+   compatible = vendor,some-iommu;
+   #iommu-cells = 1;


Troll question; what do we do when #iommu-cells  1, where the IOMMU is 
expecting some extra data associated with each ID (say, memory attributes)?


[ I'm pretty sure the answer here should be define some additional 
binding if and when anyone actually cares ;) ]



Robin.


+   };
+
+   pci: pci@f {
+   reg = 0xf 0x1;
+   compatible = vendor,pcie-root-complex;
+   device_type = pci;
+
+   /*
+* The sideband data provided to the IOMMU is the RID,
+* identity-mapped.
+*/
+   iommu-map = 0x0 iommu 0x0 0x1;
+   };
+};


--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at  

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...@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. 

Re: [PATCH/RFC 4/4] iommu/arm-smmu: Support deferred probing

[Robin Murphy]

Hi Laura,

As a heads up, I'm still vainly hoping to move the ARM SMMU driver 
entirely over to the generic framework - there's an iommu/dev branch on 
top of the iommu/dma branch I pushed earlier[1] which you might want to 
take a peek at to check if we're likely to end up pulling in different 
directions.


[1]:http://article.gmane.org/gmane.linux.kernel.iommu/8773

On 06/02/15 00:32, Laura Abbott wrote:


With the addition of clocks in the SMMU driver, the driver
now may need to be deferred if the clocks are not ready. Apart from
just the probe function though, we may need to defer attachment as
well. Support both of these.

Signed-off-by: Laura Abbott lau...@codeaurora.org
---
I went with the simplest approach ('started_probe') to indicate
we should defer probing. Another possibility I considered was to have
a 'masters added before probe completed' list and keep all masters
there until probe completes at which point we can bind the masters
to the SMMUs.


I think this looks OK for the single-distributed-SMMU case MMU-500 
allows; the Juno case with multiple MMU-401 instances is more awkward, 
but I have a feeling there's a really neat way to slot this approach to 
deferral into my per-instance stuff. I'll give this series a spin and 
see what falls out.


Thanks,
Robin.


---
  drivers/iommu/arm-smmu.c | 37 +++--
  1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index d9f7cf48..7e8194c 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -43,6 +43,8 @@
  #include linux/platform_device.h
  #include linux/slab.h
  #include linux/spinlock.h
+#include linux/of_iommu.h
+#include linux/of_platform.h

  #include linux/amba/bus.h

@@ -330,6 +332,7 @@ static int force_stage;
  module_param_named(force_stage, force_stage, int, S_IRUGO | S_IWUSR);
  MODULE_PARM_DESC(force_stage,
Force SMMU mappings to be installed at a particular stage of translation. A 
value of '1' or '2' forces the corresponding stage. All other values are ignored (i.e. no 
stage is forced). Note that selecting a specific stage will disable support for nested 
translation.);
+static bool started_probe;

  enum arm_smmu_arch_version {
ARM_SMMU_V1 = 1,
@@ -1284,7 +1287,7 @@ static int arm_smmu_attach_dev(struct iommu_domain 
*domain, struct device *dev)
smmu = find_smmu_for_device(dev);
if (!smmu) {
dev_err(dev, cannot attach to SMMU, is it on the same bus?\n);
-   return -ENXIO;
+   return started_probe ? -EPROBE_DEFER : -ENXIO;
}

if (dev-archdata.iommu) {
@@ -1677,7 +1680,7 @@ static int arm_smmu_add_device(struct device *dev)

smmu = find_smmu_for_device(dev);
if (!smmu)
-   return -ENODEV;
+   return started_probe ? -EPROBE_DEFER : -ENODEV;

group = iommu_group_alloc();
if (IS_ERR(group)) {
@@ -1761,6 +1764,11 @@ static int arm_smmu_domain_set_attr(struct iommu_domain 
*domain,
}
  }

+static int arm_smmu_of_xlate(struct device *dev, struct of_phandle_args *spec)
+{
+   return arm_smmu_add_device(dev);
+}
+
  static int arm_smmu_enable_resources(struct iommu_domain *domain)
  {
struct arm_smmu_domain *smmu_domain = domain-priv;
@@ -1814,6 +1822,7 @@ static const struct iommu_ops arm_smmu_ops = {
.pgsize_bitmap  = (SECTION_SIZE |
   ARM_SMMU_PTE_CONT_SIZE |
   PAGE_SIZE),
+   .of_xlate   = arm_smmu_of_xlate,
.enable_resources   = arm_smmu_enable_resources,
.disable_resources  = arm_smmu_disable_resources,
  };
@@ -2108,6 +2117,8 @@ static int arm_smmu_device_dt_probe(struct 
platform_device *pdev)
struct of_phandle_args masterspec;
int num_irqs, i, err;

+   started_probe = true;
+
smmu = devm_kzalloc(dev, sizeof(*smmu), GFP_KERNEL);
if (!smmu) {
dev_err(dev, failed to allocate arm_smmu_device\n);
@@ -2282,6 +2293,8 @@ static struct platform_driver arm_smmu_driver = {
.remove = arm_smmu_device_remove,
  };

+static int init_done;
+
  static int __init arm_smmu_init(void)
  {
struct device_node *np;
@@ -2316,6 +2329,7 @@ static int __init arm_smmu_init(void)
bus_set_iommu(pci_bus_type, arm_smmu_ops);
  #endif

+   init_done = true;
return 0;
  }

@@ -2327,6 +2341,25 @@ static void __exit arm_smmu_exit(void)
  subsys_initcall(arm_smmu_init);
  module_exit(arm_smmu_exit);

+static int __init arm_smmu_of_setup(struct device_node *np)
+{
+   struct platform_device *pdev;
+
+   if (!init_done)
+   arm_smmu_init();
+
+   pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
+   if (IS_ERR(pdev))
+   return PTR_ERR(pdev);
+
+   of_iommu_set_ops(np, arm_smmu_ops);
+   return 0;
+}
+

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Robin Murphy]

On 28/01/15 11:05, Catalin Marinas wrote:

On Tue, Jan 27, 2015 at 06:55:15PM +, Murali Karicheri wrote:

On 01/27/2015 06:27 AM, Robin Murphy wrote:

On 23/01/15 22:32, Murali Karicheri wrote:

Fix the dma-range size when the DT attribute is missing. i.e set size to
dev-coherent_dma_mask + 1 instead of dev-coherent_dma_mask. To detect
overflow when mask is set to max of u64, add a check, log error and
return.
Some platform use mask format for size in DTS. So add a work around to
catch this and fix.

Cc: Joerg Roedel j...@8bytes.org
Cc: Grant Likely grant.lik...@linaro.org
Cc: Rob Herring robh...@kernel.org
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Will Deacon will.dea...@arm.com
Cc: Russell King li...@arm.linux.org.uk
Cc: Arnd Bergmann a...@arndb.de
Cc: Suravee Suthikulpanit suravee.suthikulpa...@amd.com

Signed-off-by: Murali Karicheri m-kariche...@ti.com
---
drivers/of/device.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 2de320d..0a5ff54 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -105,12 +105,24 @@ void of_dma_configure(struct device *dev, struct
device_node *np)
ret = of_dma_get_range(np, dma_addr, paddr, size);
if (ret  0) {
dma_addr = offset = 0;
- size = dev-coherent_dma_mask;
+ size = dev-coherent_dma_mask + 1;
} else {
offset = PFN_DOWN(paddr - dma_addr);
+ /*
+ * Add a work around to treat the size as mask + 1 in case
+ * it is defined in DT as a mask.
+ */
+ if (size  1)
+ size = size + 1;
dev_dbg(dev, dma_pfn_offset(%#08lx)\n, offset);
}

+ /* if size is 0, we have an overflow of u64 */
+ if (!size) {
+ dev_err(dev, invalid size\n);
+ return;
+ }
+


This seems potentially fragile to dodgy DTs given that we might also be
using size to make a mask later. Would it make sense to double-up a
sanity check as mask-format detection? Something like:

if is_power_of_2(size)
// use size
else if is_power_of_2(size + 1)
// use size + 1
else
// cry


How about having the logic like this?

ret = of_dma_get_range(np, dma_addr, paddr, size);
if (ret  0) {
dma_addr = offset = 0;
size = dev-coherent_dma_mask + 1;
} else {
offset = PFN_DOWN(paddr - dma_addr);
dev_dbg(dev, dma_pfn_offset(%#08lx)\n, offset);
}

if (is_power_of_2(size + 1))
size = size + 1;
else if (!is_power_of_2(size))
{
dev_err(dev, invalid size\n);
return;
}


In of_dma_configure(), we currently assume that the default coherent
mask is 32-bit. In this thread:

http://article.gmane.org/gmane.linux.kernel/1835096

we talked about setting the coherent mask based on size automatically.
I'm not sure about the size but I think we can assume is 32-bit mask + 1
if it is not specified in the DT. Instead of just assuming a default
mask, let's assume a default size and create the mask based on this
(untested):

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 5b33c6a21807..9ff8d1286b44 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -170,10 +170,10 @@ static void of_dma_configure(struct device *dev)
struct iommu_ops *iommu;

/*
-* Set default dma-mask to 32 bit. Drivers are expected to setup
-* the correct supported dma_mask.
+* Set default size to cover the 32-bit. Drivers are expected to setup
+* the correct size and dma_mask.
 */
-   dev-coherent_dma_mask = DMA_BIT_MASK(32);
+   size = 1ULL  32;

/*
 * Set it to coherent_dma_mask by default if the architecture
@@ -185,13 +185,24 @@ static void of_dma_configure(struct device *dev)
ret = of_dma_get_range(dev-of_node, dma_addr, paddr, size);
if (ret  0) {
dma_addr = offset = 0;
-   size = dev-coherent_dma_mask;
} else {
offset = PFN_DOWN(paddr - dma_addr);
dev_dbg(dev, dma_pfn_offset(%#08lx)\n, dev-dma_pfn_offset);
}
dev-dma_pfn_offset = offset;

+   /*
+* Workaround for DTs setting the size to a mask or 0.
+*/
+   if (is_power_of_2(size + 1))
+   size += 1;


In fact, since the ilog2 below ends up effectively rounding down, we 
might as well do away with this check as well and just add 1 
unconditionally. The only time it makes any difference is when we want 
it to anyway!


I like this approach, it ends up looking a lot neater.

Robin.


+
+   /*
+* Coherent DMA masks larger than 32-bit must be explicitly set by the
+* driver.
+*/
+   dev-coherent_dma_mask = min(DMA_BIT_MASK(32), 
DMA_BIT_MASK(ilog2(size)));
+
coherent = of_dma_is_coherent(dev-of_node);
dev_dbg(dev, device is%sdma coherent\n,
coherent ?   :  not );




--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message

Re: [PATCH v5 3/8] of: fix size when dma-range is not used

[Robin Murphy]

Hi Murali,

[sorry, missed replying to yesterday's version]

On 27/01/15 21:00, Murali Karicheri wrote:

Fix the dma-range size when the DT attribute is missing. i.e  set size to
dev-coherent_dma_mask + 1 instead of dev-coherent_dma_mask. Also add
code to check invalid values of size configured in DT and log error.

Cc: Joerg Roedel j...@8bytes.org
Cc: Grant Likely grant.lik...@linaro.org
Cc: Rob Herring robh...@kernel.org
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Will Deacon will.dea...@arm.com
Cc: Russell King li...@arm.linux.org.uk
Cc: Arnd Bergmann a...@arndb.de
Cc: Suravee Suthikulpanit suravee.suthikulpa...@amd.com

Signed-off-by: Murali Karicheri m-kariche...@ti.com
---
  drivers/of/device.c |9 -
  1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 2de320d..17504f4 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -105,12 +105,19 @@ void of_dma_configure(struct device *dev, struct 
device_node *np)
ret = of_dma_get_range(np, dma_addr, paddr, size);
if (ret  0) {
dma_addr = offset = 0;
-   size = dev-coherent_dma_mask;
+   size = dev-coherent_dma_mask + 1;
} else {
offset = PFN_DOWN(paddr - dma_addr);
dev_dbg(dev, dma_pfn_offset(%#08lx)\n, offset);
}

+   if (is_power_of_2(size + 1))
+   size = size + 1;
+   else if (!is_power_of_2(size)) {
+   dev_err(dev, invalid size\n);
+   return;
+   }
+


Couldn't these checks go into the else path above? We don't need to 
check the non-DT case, because we know we've just set it to something 
sensible.


Robin.


dev-dma_pfn_offset = offset;

coherent = of_dma_is_coherent(np);




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

Re: [PATCH v4 3/6] of: fix size when dma-range is not used

[Robin Murphy]

Hi Murali,

On 23/01/15 22:32, Murali Karicheri wrote:

Fix the dma-range size when the DT attribute is missing. i.e  set size to
dev-coherent_dma_mask + 1 instead of dev-coherent_dma_mask. To detect
overflow when mask is set to max of u64, add a check, log error and return.
Some platform use mask format for size in DTS. So add a work around to
catch this and fix.

Cc: Joerg Roedel j...@8bytes.org
Cc: Grant Likely grant.lik...@linaro.org
Cc: Rob Herring robh...@kernel.org
Cc: Bjorn Helgaas bhelg...@google.com
Cc: Will Deacon will.dea...@arm.com
Cc: Russell King li...@arm.linux.org.uk
Cc: Arnd Bergmann a...@arndb.de
Cc: Suravee Suthikulpanit suravee.suthikulpa...@amd.com

Signed-off-by: Murali Karicheri m-kariche...@ti.com
---
  drivers/of/device.c |   14 +-
  1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 2de320d..0a5ff54 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -105,12 +105,24 @@ void of_dma_configure(struct device *dev, struct 
device_node *np)
ret = of_dma_get_range(np, dma_addr, paddr, size);
if (ret  0) {
dma_addr = offset = 0;
-   size = dev-coherent_dma_mask;
+   size = dev-coherent_dma_mask + 1;
} else {
offset = PFN_DOWN(paddr - dma_addr);
+   /*
+* Add a work around to treat the size as mask + 1 in case
+* it is defined in DT as a mask.
+*/
+   if (size  1)
+   size = size + 1;
dev_dbg(dev, dma_pfn_offset(%#08lx)\n, offset);
}

+   /* if size is 0, we have an overflow of u64 */
+   if (!size) {
+   dev_err(dev, invalid size\n);
+   return;
+   }
+


This seems potentially fragile to dodgy DTs given that we might also be 
using size to make a mask later. Would it make sense to double-up a 
sanity check as mask-format detection? Something like:


if is_power_of_2(size)
// use size
else if is_power_of_2(size + 1)
// use size + 1
else
// cry


Robin.


dev-dma_pfn_offset = offset;

coherent = of_dma_is_coherent(np);




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

Re: [PATCH v4 6/6] arm: dma-mapping: updates to limit dma_mask and iommu mapping size

[Robin Murphy]

Hi Murali,

On 23/01/15 22:32, Murali Karicheri wrote:

Limit the dma_mask to minimum of dma_mask and dma_base + size - 1.

Also arm_iommu_create_mapping() has size parameter of size_t and
arm_setup_iommu_dma_ops() can take a value higher than that. So
limit the size to SIZE_MAX.

Signed-off-by: Murali Karicheri m-kariche...@ti.com
---
  arch/arm/mm/dma-mapping.c |   10 ++
  1 file changed, 10 insertions(+)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 7864797..a1f9030 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2004,6 +2004,13 @@ static bool arm_setup_iommu_dma_ops(struct device *dev, 
u64 dma_base, u64 size,
if (!iommu)
return false;

+   /*
+* currently arm_iommu_create_mapping() takes a max of size_t
+* for size param. So check this limit for now.
+*/
+   if (size  SIZE_MAX)
+   return false;
+
mapping = arm_iommu_create_mapping(dev-bus, dma_base, size);
if (IS_ERR(mapping)) {
pr_warn(Failed to create %llu-byte IOMMU mapping for device 
%s\n,
@@ -2053,6 +2060,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, 
u64 size,
  {
struct dma_map_ops *dma_ops;

+   /* limit dma_mask to the lower of the two values */
+   *dev-dma_mask = min((*dev-dma_mask), (dma_base + size - 1));
+


Is there any reason not to do this in of_dma_configure? It seems like 
something everyone could benefit from - I'd cooked up a dodgy workaround 
for the same issue in my arm64 IOMMU code, but handling it generically 
in common code would be much nicer.


Robin.


dev-archdata.dma_coherent = coherent;
if (arm_setup_iommu_dma_ops(dev, dma_base, size, iommu))
dma_ops = arm_get_iommu_dma_map_ops(coherent);




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

[PATCH] arm64: dts: add baud rate to Juno stdout-path

[Robin Murphy]

Without explicit command-line parameters, the Juno UART ends up running
at 57600 baud in the kernel, which is at odds with the 115200 baud used
by the rest of the firmware. Since commit 7914a7c5651a5161 now lets us
fix this by specifying default options in stdout-path, do so.

Acked-by: Mark Rutland mark.rutl...@arm.com
Signed-off-by: Robin Murphy robin.mur...@arm.com
---
 arch/arm64/boot/dts/arm/juno.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
index cb3073e..d429129 100644
--- a/arch/arm64/boot/dts/arm/juno.dts
+++ b/arch/arm64/boot/dts/arm/juno.dts
@@ -22,7 +22,7 @@
};
 
chosen {
-   stdout-path = soc_uart0;
+   stdout-path = serial0:115200n8;
};
 
psci {
-- 
1.9.1


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

Re: [PATCH] of: amba: use of_dma_configure for AMBA devices

[Robin Murphy]

Thanks Catalin/Will/Rob,

On 17/09/14 19:05, Will Deacon wrote:

On Wed, Sep 17, 2014 at 06:47:19PM +0100, Rob Herring wrote:

On Wed, Sep 17, 2014 at 12:03 PM, Will Deacon will.dea...@arm.com wrote:

On Wed, Sep 17, 2014 at 12:56:07PM +0100, Robin Murphy wrote:

Commit 591c1e (of: configure the platform device dma parameters)
introduced a common mechanism to configure DMA from DT properties.
AMBA devices created from DT can take advantage of this, too.

Signed-off-by: Robin Murphy robin.mur...@arm.com


   Acked-by: Will Deacon will.dea...@arm.com

It would be great if the arm-soc guys can pick this up.


Is this a dependency for something else? If so,

Acked-by: Rob Herring r...@kernel.org


Yeah, it's going to be needed by my IOMMU init rework so that AMBA devices
get registered with their IOMMUs.



Noob question: Does that mean I should resend (with ACKs) to 
a...@kernel.org? (I wasn't entirely sure where this should go, hence just 
throwing at the list with CCs)


Robin.


Will




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

[PATCH] of: amba: use of_dma_configure for AMBA devices

[Robin Murphy]

Commit 591c1e (of: configure the platform device dma parameters)
introduced a common mechanism to configure DMA from DT properties.
AMBA devices created from DT can take advantage of this, too.

Signed-off-by: Robin Murphy robin.mur...@arm.com
---
 drivers/of/platform.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 0197725..3b64d0b 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -160,11 +160,10 @@ EXPORT_SYMBOL(of_device_alloc);
  * can use Platform bus notifier and handle BUS_NOTIFY_ADD_DEVICE event
  * to fix up DMA configuration.
  */
-static void of_dma_configure(struct platform_device *pdev)
+static void of_dma_configure(struct device *dev)
 {
u64 dma_addr, paddr, size;
int ret;
-   struct device *dev = pdev-dev;
 
/*
 * Set default dma-mask to 32 bit. Drivers are expected to setup
@@ -229,7 +228,7 @@ static struct platform_device 
*of_platform_device_create_pdata(
if (!dev)
goto err_clear_flag;
 
-   of_dma_configure(dev);
+   of_dma_configure(dev-dev);
dev-dev.bus = platform_bus_type;
dev-dev.platform_data = platform_data;
 
@@ -291,7 +290,6 @@ static struct amba_device *of_amba_device_create(struct 
device_node *node,
}
 
/* setup generic device info */
-   dev-dev.coherent_dma_mask = ~0;
dev-dev.of_node = of_node_get(node);
dev-dev.parent = parent;
dev-dev.platform_data = platform_data;
@@ -299,6 +297,7 @@ static struct amba_device *of_amba_device_create(struct 
device_node *node,
dev_set_name(dev-dev, %s, bus_id);
else
of_device_make_bus_id(dev-dev);
+   of_dma_configure(dev-dev);
 
/* Allow the HW Peripheral ID to be overridden */
prop = of_get_property(node, arm,primecell-periphid, NULL);
-- 
1.9.1


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