On Wed, Jan 9, 2013 at 7:23 AM, Sylwester Nawrocki
[off-list ref] wrote:

On 01/07/2013 11:45 AM, KyongHo Cho wrote:

quoted

quoted

quoted

The current exynos-iommu(System MMU) driver does not work autonomously
since it is lack of support for power management of peripheral blocks.
For example, MFC device driver must ensure that its System MMU is

disabled

quoted

quoted

before MFC block is power-down not to invalidate IOTLB in the System
MMU
when I/O memory mapping is changed. Because A System MMU is resides

in the

quoted

quoted

same H/W block, access to control registers of System MMU while the H/W
block is turned off must be prohibited.

This set of changes solves the above problem with setting each System

MMUs

quoted

quoted

as the parent of the device which owns the System MMU to recieve the
information when the device is turned off or turned on.

 I intend to make Exynos4412 FIMC-LITEn (Exynos5 CAMIFn) devices child
 devices of the FIMC-IS (camera ISP) platform device. This patch reflects
 that: http://patchwork.linuxtv.org/patch/16046

 This is required since AFAIK FIMC-LITE depends on clocks from FIMC-IS.
 By setting fimc-is device as the parent fimc-lite's dependency on it is
 resolved without any hacks between these drivers.

 Then how this tree will look like after your sysmmu related
re-parenting:

          fimc-is
         /      \
  fimc-lite0    fimc-lite1


 ?

First of all, I think that clock dependency shuold be resolved with
setting the parent of clock descriptor of fimc-lite  to the clock
descriptor of fimc-is.

I'll need to investigate it more, but AFAIU there is more than one clock
for the FIMC-IS device that needs to be enabled before FIMC-LITE can be
used. IOW FIMC-LITE must be activated after FIMC-IS, and deactivated before
FIMC-IS is (runtime) suspended.

So I'm afraid I can't simply alter the clock tree for the sake of the
subsystem dependencies - it's not a one-to-one relation and it doesn't
sound right.

I have just little knowledge about FIMC-IS.
I don't understand why the sequence of power gating or suspend/resume
is important.
Are you concerning about the dependency of clock gating?
If the drivers of FIMC-IS and FIMC-LITE are not dependent upon each other,
I think it is just enough to add them to the same power domain.

I will check the clock description in the devicetree.

quoted

If you are concerning about power management, it is simply resolved with
putting fimc-lite to the power domain of fimc-is.

Yes, these devices are already registered to same power domain (ISP).

quoted

The above tree will be changed like below after probing System MMU:
sysmmu-fimc-is
             I
      fimc-is

sysmmu-fimc-lite0
               I
       fimc-lite0

sysmmu-fimc-lite1
              I
        fimc-lite1

I'm just concerned that this iommu driver would drop previous parent
configurations and introduce its own. There might be other devices for
which this would be harmful. Didn't you consider just moving any existing
device's parent and setting it as iommu's parent, so the tree looks like

         sysmmu-fimc-is
               |
            fimc-is
        /            \
  sysmmu-fimc-lite0  sysmmu-fimc-lite1
        |              |
  fimc-lite0         fimc-lite1

?

But it's not really much better...

Thanks for the proposal. I will consider to insert System MMU in the
existing dpm and rpm tree
without breaking the existing tree.

I did not find a better solution to guarantee that the System MMU is
always resumed
before its master (like fimc-lite0) is resumed and suspended after its
master is suspended.
It must be guaranteed in terms of APM and Runtime PM.

quoted

quoted

quoted

Another big change to the driver is the support for devicetree.
The bindings for System MMU is described in
Documentation/devicetree/bindings/arm/samsung/system-mmu.txt

 Yes, and there is no patch adding this file in this series. Let me paste
 it here:

 From 88987ff5b77acc7211b9f537a6ef6ea38e3efdd0 Mon Sep 17 00:00:00 2001
 From: KyongHo Cho <pullip.cho@samsung.com
<mailto:pullip.cho@samsung.com>>

 Date: Tue, 11 Dec 2012 14:06:25 +0900
 Subject: [PATCH] ARM: EXYNOS: add System MMU definition to DT

 This commit adds System MMU nodes to DT of Exynos SoCs.

 Signed-off-by: KyongHo Cho <pullip.cho@samsung.com

<mailto:pullip.cho@samsung.com>>

quoted

 Signed-off-by: Kukjin Kim <kgene.kim@samsung.com

<mailto:kgene.kim@samsung.com>>

quoted

 ---
  .../devicetree/bindings/arm/exynos/system-mmu.txt  |   86 ++++++++++++
  arch/arm/boot/dts/exynos4210.dtsi                  |   96 +++++++++++++
  arch/arm/boot/dts/exynos4x12.dtsi                  |  124

+++++++++++++++++

quoted

  arch/arm/boot/dts/exynos5250.dtsi                  |  147

+++++++++++++++++++-

quoted

  4 files changed, 451 insertions(+), 2 deletions(-)
  create mode 100644

Documentation/devicetree/bindings/arm/exynos/system-mmu.txt

quoted

 diff --git

a/Documentation/devicetree/bindings/arm/exynos/system-mmu.txt
b/Documentation/devicetree/bindings/arm/exynos/system-mmu.txt

quoted

 new file mode 100644
 index 0000000..b33d682
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/arm/exynos/system-mmu.txt
 @@ -0,0 +1,86 @@
 +* Samsung Exynos System MMU
 +
 +Samsung's Exynos architecture includes System MMU that enables
scattered
 +physical chunks to be visible as a contiguous region to DMA-capabile

peripheral

quoted

 +devices like MFC, FIMC, FIMD, GScaler, FIMC-IS and so forth.
 +
 +System MMU is a sort of IOMMU and support identical translation table

format to

quoted

 +ARMv7 translation tables with minimum set of page properties

including access

quoted

 +permissions, shareability and security protection. In addition System

MMU has

quoted

 +another capabilities like L2 TLB or block-fetch buffers to minimize

translation

quoted

 +latency
 +
 +Each System MMU is included in the H/W block of a peripheral device.

Thus, it is

quoted

 +important to specify that a System MMU is dedicated to which

peripheral device

quoted

 +before using System MMU. System initialization must specify the

relationships

quoted

 +between a System MMU and a peripheral device that owns the System MMU.
 +
 +Some device drivers may control several peripheral devices with a

single device

quoted

 +descriptor like MFC. Since handling a System MMU with IOMMU API

requires a

quoted

 +device descriptor that needs the System MMU, it is best to combine

the System

quoted

 +MMUs of the peripheral devices and control them with a single System

MMU device

quoted

 +descriptor. If it is unable to combine them into a single device

descriptor,

quoted

 +they can be linked with each other by the means of device.parent

relationship.

quoted

 +
 +Required properties:
 +- compatible: Should be "samsung,exynos-sysmmu".
 +- reg: Tuples of base address and size of System MMU registers. The

number of

quoted

 +       tuples can be more than one if two or more System MMUs are

controlled

quoted

 +       by a single device descriptor.
 +- interrupt-parent: The phandle of the interrupt controller of System
MMU
 +- interrupts: Tuples of numbers that indicates the interrupt source.
The
 +              number of elements in the tuple is dependent upon
 + 'interrupt-parent' property. The number of tuples in this property
 +             must be the same with 'reg' property.
 +
 +Optional properties:
 +- mmuname: Strings of the name of System MMU for debugging purpose.

The number

quoted

 +          of strings must be the same with the number of tuples in
'reg'
 +          property.

 As commented on previous patch, this isn't something that belongs here.
 But for debugging you could probably retrieve this from the node name ?

Thank you for good idea. However mmuname is an array of strings,
actually. Anyway I agree with your opinion that 'mmuname' is not proper
property of a device node. I will remove it and substitute it with base
register address of a System MMU.

Ok.

quoted

quoted

 +- mmu-master: phandle to the device node that owns System MMU. Only

the device

quoted

 +          that is specified whith this property can control System

MMU with

quoted

 +          IOMMU API.
 +
 +Examples:
 +
 +MFC has 2 System MMUs for each port that MFC is attached. Thus it

seems natural

quoted

 +to define 2 System MMUs for each port of the MFC:

"The video codec (MFC) device has a System MMUs for each port (AXI master).
Thus it seems natural to define a System MMU device node for each port of

the MFC:"

Thanks. I think your expression is easier to understand.
I am also not a good English writer :)

quoted

quoted

 +
 +       sysmmu-mfc-l {
 +               mmuname = "mfc_l";
 +               reg = <0x11210000 0x1000>;
 +               compatible = "samsung,exynos-sysmmu";
 +               interrupt-parent = <&combiner>;
 +               interrupts = <8 5>;
 +               mmu-master = <&mfc>;
 +       };
 +
 +       sysmmu-mfc-r {
 +               mmuname = "mfc_r";
 +               reg = <0x11200000 0x1000>;
 +               compatible = "samsung,exynos-sysmmu";
 +               interrupt-parent = <&combiner>;
 +               interrupts = <6 2>;
 +               mmu-master = <&mfc>;
 +       };
 +
 +Actually, MFC device driver requires sub-devices that represents each

port and

quoted

 +above 'mmu-master' properties of sysmmu-mfc-l and sysmmu-mfc-r have

the phandles

quoted

 +to those sub-devices.

 I find this sentence really difficult to parse. This documentation

should talk

quoted

 about how the device is designed and represented, rather than about

its driver.

quoted

OK. I will trying to find another expression easy to understand.  Do you
have any suggestion?

I'm not a native English speaker, but maybe something like this makes sense:

"The sysmmu-mfc-l, sysmmy-mfc-r nodes represent parts of the MFC device
which
indicate their 'mmu-master' phandles pointing to the mfc node."

I understand what you have intended.
Thank you for the suggestion. I will consider to make it easy to understand.

?

quoted

quoted

 +However, it is also a good idea that treats the above System MMUs as

treats -> treat

Thanks.

quoted

one System

quoted

 +MMU because those System MMUs are actually required by the MFC device:
 +
 +       sysmmu-mfc {
 +               mmuname = "mfc_l", "mfc_r";
 +               reg = <0x11210000 0x1000
 +                      0x11200000 0x1000>;
 +               compatible = "samsung,exynos-sysmmu";
 +               interrupt-parent = <&combiner>;
 +               interrupts = <8 5
 +                             6 2>;

interrupts = <8 5>, <6 2>; ?

Please see my reply below.

quoted

quoted

 +               mmu-master = <&mfc>;
 +       };
 +
 +If System MMU of MFC is defined like the above, the number of

elements and the

quoted

 +order of list in 'mmuname', 'reg' and 'interrupts' must be the same.

...

quoted

quoted

 diff --git a/arch/arm/boot/dts/exynos5250.dtsi

b/arch/arm/boot/dts/exynos5250.dtsi

quoted

 index 2e3b6ef..2ff6d78 100644
 --- a/arch/arm/boot/dts/exynos5250.dtsi
 +++ b/arch/arm/boot/dts/exynos5250.dtsi
 @@ -75,7 +75,7 @@
                 interrupts = <0 42 0>;
         };

...

quoted

quoted

 +       sysmmu-is0 {
 +               mmuname = "isp", "drc", "scalerc", "scalerp", "fd",
"mcu";
 +               reg = < 0x13260000 0x1000
 +                       0x13270000 0x1000
 +                       0x13280000 0x1000
 +                       0x13290000 0x1000
 +                       0x132A0000 0x1000
 +                       0x132B0000 0x1000 >;
 +               compatible = "samsung,exynos-sysmmu";
 +               interrupt-parent = <&combiner>;
 +               interrupts = <  10 6
 +                               11 6
 +                                5 2
 +                                3 6
 +                                5 0
 +                                5 4 >;

 I believe this should be

                 interrupts = <10 6>, <11 6>, <5 2>,
 <3 6>, <5 0>, <5 4>;

I found the syntax of array of resources in the specifications of device
tree. I found that it works correctly.

OK, it seems both conventions are valid. I just found it unusual, since
all interrupt specifiers I've seen for Samsung SoCs use the syntax, where
each interrupt is enclosed in triangular brackets. Maybe it's better to
keep it consistent across all files ?

Let me check the other dts/dti and change my expression to follow the
convention:)


Sorry for late reply. I have just little time to spend after work in
these days. :(

Thank you for kind review.

KyongHo