Re: [PATCH] ARM: EXYNOS: Add MFC device tree support

Re: [PATCH] ARM: EXYNOS: Add MFC device tree support

[Thomas Abraham]

On 16 August 2012 18:01, Arun Kumar K <arun.kk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> From: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>
> This patch adds device tree entry for MFC in the Exynos
> machines. Exynos4 SoCs support MFC v5 version and Exynos5 has
> MFC v6.x version. So making the required changes in the clock
> files and adds MFC to the DT device list.
>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Arun Kumar K <arun.kk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  .../devicetree/bindings/media/s5p-mfc.txt          |   24 ++++++++++++++++++++
>  arch/arm/boot/dts/exynos4210.dtsi                  |   10 ++++++++
>  arch/arm/boot/dts/exynos5250.dtsi                  |   10 ++++++++
>  arch/arm/mach-exynos/clock-exynos5.c               |    2 +-
>  arch/arm/mach-exynos/mach-exynos4-dt.c             |   22 ++++++++++++++++++
>  arch/arm/mach-exynos/mach-exynos5-dt.c             |   22 ++++++++++++++++++
>  6 files changed, 89 insertions(+), 1 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/s5p-mfc.txt
>
> diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> new file mode 100644
> index 0000000..b9bd266
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> @@ -0,0 +1,24 @@
> +* Samsung Multi Format Codec (MFC)
> +
> +Mult Format Codec (MFC) is the IP present in Samsung SoCs which
> +supports high resolution decoding and encoding functionalities.

In addition to this, specifying that mfc is used for video
encode/decode would be informative.

> +
> +Required properties:
> +  - compatible : value should be either one among the following
> +       (a) "samsung,s5p-mfc-v5" for MFC v5 present in Exynos4 SoCs
> +       (b) "samsung,s5p-mfc-v6" for MFC v6.x present in Exynos5 SoCs

"s5p" should be dropped from the compatible values. For example, it
should be "samsung,mfc-v5", which is sufficient to identify the
version of the mfc controller.

> +
> +  - reg : Physical base address of the IP registers and length of memory
> +         mapped region.
> +
> +  - interrupts : MFC interupt number to the CPU.
> +
> +  - samsung,mfc-r : Base address of the first memory bank used by MFC
> +                   for DMA contiguous memory allocation.
> +
> +  - samsung,mfc-r-size : Size of the first memory bank.

It is not allowed to pass buffer base address and size from device
tree. Device tree node should describe only the MFC controller
hardware. Any memory management related information should be handled
outside of device tree. This helps the bindings to be reusable across
multiple operating systems.

> +
> +  - samsung,mfc-l : Base address of the second memory bank used by MFC
> +                   for DMA contiguous memory allocation.
> +
> +  - samsung,mfc-l-size : Size of the second memory bank.

Same comment as above. And the bindings documentation is usually
included in the same patch that adds device tree support for the
driver.

> diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
> index 02891fe..b5ee43d 100644
> --- a/arch/arm/boot/dts/exynos4210.dtsi
> +++ b/arch/arm/boot/dts/exynos4210.dtsi
> @@ -56,6 +56,16 @@
>                 interrupts = <0 43 0>;
>         };
>
> +       mfc {
> +               compatible = "samsung,s5p-mfc";
> +               reg = <0x13400000 0x10000>;
> +               interrupts = <0 94 0>;
> +               samsung,mfc-r = <0x43000000>;
> +               samsung,mfc-r-size = <8388608>;
> +               samsung,mfc-l = <0x51000000>;
> +               samsung,mfc-l-size = <8388608>;
> +       };
> +
>         rtc@10070000 {
>                 compatible = "samsung,s3c6410-rtc";
>                 reg = <0x10070000 0x100>;
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 004aaa8..3c762a4 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -58,6 +58,16 @@
>                 interrupts = <0 42 0>;
>         };
>
> +       mfc {
> +               compatible = "samsung,s5p-mfc-v6";
> +               reg = <0x11000000 0x10000>;
> +               interrupts = <0 96 0>;
> +               samsung,mfc-r = <0x43000000>;
> +               samsung,mfc-r-size = <8388608>;
> +               samsung,mfc-l = <0x51000000>;
> +               samsung,mfc-l-size = <8388608>;
> +       };
> +
>         rtc {
>                 compatible = "samsung,s3c6410-rtc";
>                 reg = <0x101E0000 0x100>;
> diff --git a/arch/arm/mach-exynos/clock-exynos5.c b/arch/arm/mach-exynos/clock-exynos5.c
> index 3b00e29..c85e7b2 100644
> --- a/arch/arm/mach-exynos/clock-exynos5.c
> +++ b/arch/arm/mach-exynos/clock-exynos5.c
> @@ -607,7 +607,7 @@ static struct clk exynos5_init_clocks_off[] = {
>                 .ctrlbit        = (1 << 25),
>         }, {
>                 .name           = "mfc",
> -               .devname        = "s5p-mfc",
> +               .devname        = "s5p-mfc-v6",
>                 .enable         = exynos5_clk_ip_mfc_ctrl,
>                 .ctrlbit        = (1 << 0),
>         }, {
> diff --git a/arch/arm/mach-exynos/mach-exynos4-dt.c b/arch/arm/mach-exynos/mach-exynos4-dt.c
> index b2b5d5f..c4a0e16 100644
> --- a/arch/arm/mach-exynos/mach-exynos4-dt.c
> +++ b/arch/arm/mach-exynos/mach-exynos4-dt.c
> @@ -13,6 +13,7 @@
>
>  #include <linux/of_platform.h>
>  #include <linux/serial_core.h>
> +#include <linux/memblock.h>
>
>  #include <asm/mach/arch.h>
>  #include <asm/hardware/gic.h>
> @@ -63,6 +64,7 @@ static const struct of_dev_auxdata exynos4210_auxdata_lookup[] __initconst = {
>                                 "exynos4210-spi.2", NULL),
>         OF_DEV_AUXDATA("arm,pl330", EXYNOS4_PA_PDMA0, "dma-pl330.0", NULL),
>         OF_DEV_AUXDATA("arm,pl330", EXYNOS4_PA_PDMA1, "dma-pl330.1", NULL),
> +       OF_DEV_AUXDATA("samsung,s5p-mfc", 0x13400000, "s5p-mfc", NULL),
>         {},
>  };
>
> @@ -83,6 +85,25 @@ static char const *exynos4210_dt_compat[] __initdata = {
>         NULL
>  };
>
> +static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
> +               phys_addr_t lbase, unsigned int lsize) {
> +
> +       if (memblock_remove(lbase, lsize)) {
> +               pr_err("Failed to reserve bank1 memory for MFC device\n");
> +               WARN_ON(1);
> +       }
> +
> +       if (memblock_remove(rbase, rsize)) {
> +               pr_err("Failed to reserve bank2 memory for MFC device\n");
> +               WARN_ON(1);
> +       }
> +}

It would be better to split the device tree related changes and the
driver buffer management related changes in separate patches.

> +
> +static void __init exynos4_reserve(void)
> +{
> +       s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
> +}
> +
>  DT_MACHINE_START(EXYNOS4210_DT, "Samsung Exynos4 (Flattened Device Tree)")
>         /* Maintainer: Thomas Abraham <thomas.abraham-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> */
>         .init_irq       = exynos4_init_irq,
> @@ -93,4 +114,5 @@ DT_MACHINE_START(EXYNOS4210_DT, "Samsung Exynos4 (Flattened Device Tree)")
>         .timer          = &exynos4_timer,
>         .dt_compat      = exynos4210_dt_compat,
>         .restart        = exynos4_restart,
> +       .reserve        = exynos4_reserve,
>  MACHINE_END
> diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c b/arch/arm/mach-exynos/mach-exynos5-dt.c
> index ef770bc..898d2de 100644
> --- a/arch/arm/mach-exynos/mach-exynos5-dt.c
> +++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
> @@ -11,6 +11,7 @@
>
>  #include <linux/of_platform.h>
>  #include <linux/serial_core.h>
> +#include <linux/memblock.h>
>
>  #include <asm/mach/arch.h>
>  #include <asm/hardware/gic.h>
> @@ -56,6 +57,7 @@ static const struct of_dev_auxdata exynos5250_auxdata_lookup[] __initconst = {
>         OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_PDMA0, "dma-pl330.0", NULL),
>         OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_PDMA1, "dma-pl330.1", NULL),
>         OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_MDMA1, "dma-pl330.2", NULL),
> +       OF_DEV_AUXDATA("samsung,s5p-mfc-v6", 0x11000000, "s5p-mfc-v6", NULL),
>         {},
>  };
>
> @@ -76,6 +78,25 @@ static char const *exynos5250_dt_compat[] __initdata = {
>         NULL
>  };
>
> +static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
> +               phys_addr_t lbase, unsigned int lsize) {
> +
> +       if (memblock_remove(lbase, lsize)) {
> +               pr_err("Failed to reserve bank1 memory for MFC device\n");
> +               WARN_ON(1);
> +       }
> +
> +       if (memblock_remove(rbase, rsize)) {
> +               pr_err("Failed to reserve bank2 memory for MFC device\n");
> +               WARN_ON(1);
> +       }
> +}
> +
> +static void __init exynos5_reserve(void)
> +{
> +       s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
> +}
> +
>  DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5 (Flattened Device Tree)")
>         /* Maintainer: Kukjin Kim <kgene.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> */
>         .init_irq       = exynos5_init_irq,
> @@ -86,4 +107,5 @@ DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5 (Flattened Device Tree)")
>         .timer          = &exynos4_timer,
>         .dt_compat      = exynos5250_dt_compat,
>         .restart        = exynos5_restart,
> +       .reserve        = exynos5_reserve,
>  MACHINE_END
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ARM: EXYNOS: Add MFC device tree support

[Arun Kumar K]

From: Naveen Krishna Chatradhi <ch.naveen@samsung.com>

This patch adds device tree entry for MFC in the Exynos
machines. Exynos4 SoCs support MFC v5 version and Exynos5 has
MFC v6.x version. So making the required changes in the clock
files and adds MFC to the DT device list.

Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
---
 .../devicetree/bindings/media/s5p-mfc.txt          |   24 ++++++++++++++++++++
 arch/arm/boot/dts/exynos4210.dtsi                  |   10 ++++++++
 arch/arm/boot/dts/exynos5250.dtsi                  |   10 ++++++++
 arch/arm/mach-exynos/clock-exynos5.c               |    2 +-
 arch/arm/mach-exynos/mach-exynos4-dt.c             |   22 ++++++++++++++++++
 arch/arm/mach-exynos/mach-exynos5-dt.c             |   22 ++++++++++++++++++
 6 files changed, 89 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/media/s5p-mfc.txt

diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt
new file mode 100644
index 0000000..b9bd266
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
@@ -0,0 +1,24 @@
+* Samsung Multi Format Codec (MFC)
+
+Mult Format Codec (MFC) is the IP present in Samsung SoCs which
+supports high resolution decoding and encoding functionalities.
+
+Required properties:
+  - compatible : value should be either one among the following
+	(a) "samsung,s5p-mfc-v5" for MFC v5 present in Exynos4 SoCs
+	(b) "samsung,s5p-mfc-v6" for MFC v6.x present in Exynos5 SoCs
+
+  - reg : Physical base address of the IP registers and length of memory
+	  mapped region.
+
+  - interrupts : MFC interupt number to the CPU.
+
+  - samsung,mfc-r : Base address of the first memory bank used by MFC
+		    for DMA contiguous memory allocation.
+
+  - samsung,mfc-r-size : Size of the first memory bank.
+
+  - samsung,mfc-l : Base address of the second memory bank used by MFC
+		    for DMA contiguous memory allocation.
+
+  - samsung,mfc-l-size : Size of the second memory bank.
diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
index 02891fe..b5ee43d 100644
--- a/arch/arm/boot/dts/exynos4210.dtsi
+++ b/arch/arm/boot/dts/exynos4210.dtsi
@@ -56,6 +56,16 @@
 		interrupts = <0 43 0>;
 	};
 
+	mfc {
+		compatible = "samsung,s5p-mfc";
+		reg = <0x13400000 0x10000>;
+		interrupts = <0 94 0>;
+		samsung,mfc-r = <0x43000000>;
+		samsung,mfc-r-size = <8388608>;
+		samsung,mfc-l = <0x51000000>;
+		samsung,mfc-l-size = <8388608>;
+	};
+
 	rtc@10070000 {
 		compatible = "samsung,s3c6410-rtc";
 		reg = <0x10070000 0x100>;
diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 004aaa8..3c762a4 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -58,6 +58,16 @@
 		interrupts = <0 42 0>;
 	};
 
+	mfc {
+		compatible = "samsung,s5p-mfc-v6";
+		reg = <0x11000000 0x10000>;
+		interrupts = <0 96 0>;
+		samsung,mfc-r = <0x43000000>;
+		samsung,mfc-r-size = <8388608>;
+		samsung,mfc-l = <0x51000000>;
+		samsung,mfc-l-size = <8388608>;
+	};
+
 	rtc {
 		compatible = "samsung,s3c6410-rtc";
 		reg = <0x101E0000 0x100>;
diff --git a/arch/arm/mach-exynos/clock-exynos5.c b/arch/arm/mach-exynos/clock-exynos5.c
index 3b00e29..c85e7b2 100644
--- a/arch/arm/mach-exynos/clock-exynos5.c
+++ b/arch/arm/mach-exynos/clock-exynos5.c
@@ -607,7 +607,7 @@ static struct clk exynos5_init_clocks_off[] = {
 		.ctrlbit	= (1 << 25),
 	}, {
 		.name		= "mfc",
-		.devname	= "s5p-mfc",
+		.devname	= "s5p-mfc-v6",
 		.enable		= exynos5_clk_ip_mfc_ctrl,
 		.ctrlbit	= (1 << 0),
 	}, {
diff --git a/arch/arm/mach-exynos/mach-exynos4-dt.c b/arch/arm/mach-exynos/mach-exynos4-dt.c
index b2b5d5f..c4a0e16 100644
--- a/arch/arm/mach-exynos/mach-exynos4-dt.c
+++ b/arch/arm/mach-exynos/mach-exynos4-dt.c
@@ -13,6 +13,7 @@
 
 #include <linux/of_platform.h>
 #include <linux/serial_core.h>
+#include <linux/memblock.h>
 
 #include <asm/mach/arch.h>
 #include <asm/hardware/gic.h>
@@ -63,6 +64,7 @@ static const struct of_dev_auxdata exynos4210_auxdata_lookup[] __initconst = {
 				"exynos4210-spi.2", NULL),
 	OF_DEV_AUXDATA("arm,pl330", EXYNOS4_PA_PDMA0, "dma-pl330.0", NULL),
 	OF_DEV_AUXDATA("arm,pl330", EXYNOS4_PA_PDMA1, "dma-pl330.1", NULL),
+	OF_DEV_AUXDATA("samsung,s5p-mfc", 0x13400000, "s5p-mfc", NULL),
 	{},
 };
 
@@ -83,6 +85,25 @@ static char const *exynos4210_dt_compat[] __initdata = {
 	NULL
 };
 
+static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
+		phys_addr_t lbase, unsigned int lsize) {
+
+	if (memblock_remove(lbase, lsize)) {
+		pr_err("Failed to reserve bank1 memory for MFC device\n");
+		WARN_ON(1);
+	}
+
+	if (memblock_remove(rbase, rsize)) {
+		pr_err("Failed to reserve bank2 memory for MFC device\n");
+		WARN_ON(1);
+	}
+}
+
+static void __init exynos4_reserve(void)
+{
+	s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
+}
+
 DT_MACHINE_START(EXYNOS4210_DT, "Samsung Exynos4 (Flattened Device Tree)")
 	/* Maintainer: Thomas Abraham <thomas.abraham@linaro.org> */
 	.init_irq	= exynos4_init_irq,
@@ -93,4 +114,5 @@ DT_MACHINE_START(EXYNOS4210_DT, "Samsung Exynos4 (Flattened Device Tree)")
 	.timer		= &exynos4_timer,
 	.dt_compat	= exynos4210_dt_compat,
 	.restart        = exynos4_restart,
+	.reserve	= exynos4_reserve,
 MACHINE_END
diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c b/arch/arm/mach-exynos/mach-exynos5-dt.c
index ef770bc..898d2de 100644
--- a/arch/arm/mach-exynos/mach-exynos5-dt.c
+++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
@@ -11,6 +11,7 @@
 
 #include <linux/of_platform.h>
 #include <linux/serial_core.h>
+#include <linux/memblock.h>
 
 #include <asm/mach/arch.h>
 #include <asm/hardware/gic.h>
@@ -56,6 +57,7 @@ static const struct of_dev_auxdata exynos5250_auxdata_lookup[] __initconst = {
 	OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_PDMA0, "dma-pl330.0", NULL),
 	OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_PDMA1, "dma-pl330.1", NULL),
 	OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_MDMA1, "dma-pl330.2", NULL),
+	OF_DEV_AUXDATA("samsung,s5p-mfc-v6", 0x11000000, "s5p-mfc-v6", NULL),
 	{},
 };
 
@@ -76,6 +78,25 @@ static char const *exynos5250_dt_compat[] __initdata = {
 	NULL
 };
 
+static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
+		phys_addr_t lbase, unsigned int lsize) {
+
+	if (memblock_remove(lbase, lsize)) {
+		pr_err("Failed to reserve bank1 memory for MFC device\n");
+		WARN_ON(1);
+	}
+
+	if (memblock_remove(rbase, rsize)) {
+		pr_err("Failed to reserve bank2 memory for MFC device\n");
+		WARN_ON(1);
+	}
+}
+
+static void __init exynos5_reserve(void)
+{
+	s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
+}
+
 DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5 (Flattened Device Tree)")
 	/* Maintainer: Kukjin Kim <kgene.kim@samsung.com> */
 	.init_irq	= exynos5_init_irq,
@@ -86,4 +107,5 @@ DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5 (Flattened Device Tree)")
 	.timer		= &exynos4_timer,
 	.dt_compat	= exynos5250_dt_compat,
 	.restart        = exynos5_restart,
+	.reserve	= exynos5_reserve,
 MACHINE_END
-- 
1.7.0.4

Re: [PATCH] ARM: EXYNOS: Add MFC device tree support

[Arun Kumar K]

Hi Thomas,
Thank you for the comments.
Please find my response inline.

------- Original Message -------
Sender : Thomas Abraham<thomas.abraham@linaro.org> 
Date   : Aug 16, 2012 17:12 (GMT+05:30)
Title  : Re: [PATCH] ARM: EXYNOS: Add MFC device tree support

On 16 August 2012 18:01, Arun Kumar K <arun.kk@samsung.com> wrote:
> From: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
>
> This patch adds device tree entry for MFC in the Exynos
> machines. Exynos4 SoCs support MFC v5 version and Exynos5 has
> MFC v6.x version. So making the required changes in the clock
> files and adds MFC to the DT device list.
>
> Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> ---
>  .../devicetree/bindings/media/s5p-mfc.txt          |   24 ++++++++++++++++++++
>  arch/arm/boot/dts/exynos4210.dtsi                  |   10 ++++++++
>  arch/arm/boot/dts/exynos5250.dtsi                  |   10 ++++++++
>  arch/arm/mach-exynos/clock-exynos5.c               |    2 +-
>  arch/arm/mach-exynos/mach-exynos4-dt.c             |   22 ++++++++++++++++++
>  arch/arm/mach-exynos/mach-exynos5-dt.c             |   22 ++++++++++++++++++
>  6 files changed, 89 insertions(+), 1 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/media/s5p-mfc.txt
>
> diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> new file mode 100644
> index 0000000..b9bd266
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> @@ -0,0 +1,24 @@
> +* Samsung Multi Format Codec (MFC)
> +
> +Mult Format Codec (MFC) is the IP present in Samsung SoCs which
> +supports high resolution decoding and encoding functionalities.
>
> In addition to this, specifying that mfc is used for video
> encode/decode would be informative.

Ok. I will make it more descriptive.

> +
> +Required properties:
> +  - compatible : value should be either one among the following
> +       (a) "samsung,s5p-mfc-v5" for MFC v5 present in Exynos4 SoCs
> +       (b) "samsung,s5p-mfc-v6" for MFC v6.x present in Exynos5 SoCs
>
> "s5p" should be dropped from the compatible values. For example, it
> should be "samsung,mfc-v5", which is sufficient to identify the
> version of the mfc controller.

Ok will remove s5p.

> +
> +  - reg : Physical base address of the IP registers and length of memory
> +         mapped region.
> +
> +  - interrupts : MFC interupt number to the CPU.
> +
> +  - samsung,mfc-r : Base address of the first memory bank used by MFC
> +                   for DMA contiguous memory allocation.
> +
> +  - samsung,mfc-r-size : Size of the first memory bank.
>
> It is not allowed to pass buffer base address and size from device
> tree. Device tree node should describe only the MFC controller
> hardware. Any memory management related information should be handled
> outside of device tree. This helps the bindings to be reusable across
> multiple operating systems.

The mfc-l and mfc-r base address and size is board specific info which has to
be passed to the driver. This is used for DMA contiguous allocation by driver and this value
can change on a different board. 
So in that case, i will pass it as platform data to the driver from mach-exynos5-dt.c file.
I hope that would be ok.

> +
> +  - samsung,mfc-l : Base address of the second memory bank used by MFC
> +                   for DMA contiguous memory allocation.
> +
> +  - samsung,mfc-l-size : Size of the second memory bank.
>
> Same comment as above. And the bindings documentation is usually
> included in the same patch that adds device tree support for the
> driver.

Ok

> diff --git a/arch/arm/boot/dts/exynos4210.dtsi b/arch/arm/boot/dts/exynos4210.dtsi
> index 02891fe..b5ee43d 100644
> --- a/arch/arm/boot/dts/exynos4210.dtsi
> +++ b/arch/arm/boot/dts/exynos4210.dtsi
> @@ -56,6 +56,16 @@
>                 interrupts = <0 43 0>;
>         };
>
> +       mfc {
> +               compatible = "samsung,s5p-mfc";
> +               reg = <0x13400000 0x10000>;
> +               interrupts = <0 94 0>;
> +               samsung,mfc-r = <0x43000000>;
> +               samsung,mfc-r-size = <8388608>;
> +               samsung,mfc-l = <0x51000000>;
> +               samsung,mfc-l-size = <8388608>;
> +       };
> +
>         rtc@10070000 {
>                 compatible = "samsung,s3c6410-rtc";
>                 reg = <0x10070000 0x100>;
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 004aaa8..3c762a4 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -58,6 +58,16 @@
>                 interrupts = <0 42 0>;
>         };
>
> +       mfc {
> +               compatible = "samsung,s5p-mfc-v6";
> +               reg = <0x11000000 0x10000>;
> +               interrupts = <0 96 0>;
> +               samsung,mfc-r = <0x43000000>;
> +               samsung,mfc-r-size = <8388608>;
> +               samsung,mfc-l = <0x51000000>;
> +               samsung,mfc-l-size = <8388608>;
> +       };
> +
>         rtc {
>                 compatible = "samsung,s3c6410-rtc";
>                 reg = <0x101E0000 0x100>;
> diff --git a/arch/arm/mach-exynos/clock-exynos5.c b/arch/arm/mach-exynos/clock-exynos5.c
> index 3b00e29..c85e7b2 100644
> --- a/arch/arm/mach-exynos/clock-exynos5.c
> +++ b/arch/arm/mach-exynos/clock-exynos5.c
> @@ -607,7 +607,7 @@ static struct clk exynos5_init_clocks_off[] = {
>                 .ctrlbit        = (1 << 25),
>         }, {
>                 .name           = "mfc",
> -               .devname        = "s5p-mfc",
> +               .devname        = "s5p-mfc-v6",
>                 .enable         = exynos5_clk_ip_mfc_ctrl,
>                 .ctrlbit        = (1 << 0),
>         }, {
> diff --git a/arch/arm/mach-exynos/mach-exynos4-dt.c b/arch/arm/mach-exynos/mach-exynos4-dt.c
> index b2b5d5f..c4a0e16 100644
> --- a/arch/arm/mach-exynos/mach-exynos4-dt.c
> +++ b/arch/arm/mach-exynos/mach-exynos4-dt.c
> @@ -13,6 +13,7 @@
>
>  #include <linux/of_platform.h>
>  #include <linux/serial_core.h>
> +#include <linux/memblock.h>
>
>  #include <asm/mach/arch.h>
>  #include <asm/hardware/gic.h>
> @@ -63,6 +64,7 @@ static const struct of_dev_auxdata exynos4210_auxdata_lookup[] __initconst = {
>                                 "exynos4210-spi.2", NULL),
>         OF_DEV_AUXDATA("arm,pl330", EXYNOS4_PA_PDMA0, "dma-pl330.0", NULL),
>         OF_DEV_AUXDATA("arm,pl330", EXYNOS4_PA_PDMA1, "dma-pl330.1", NULL),
> +       OF_DEV_AUXDATA("samsung,s5p-mfc", 0x13400000, "s5p-mfc", NULL),
>         {},
>  };
>
> @@ -83,6 +85,25 @@ static char const *exynos4210_dt_compat[] __initdata = {
>         NULL
>  };
>
> +static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
> +               phys_addr_t lbase, unsigned int lsize) {
> +
> +       if (memblock_remove(lbase, lsize)) {
> +               pr_err("Failed to reserve bank1 memory for MFC device
");
> +               WARN_ON(1);
> +       }
> +
> +       if (memblock_remove(rbase, rsize)) {
> +               pr_err("Failed to reserve bank2 memory for MFC device
");
> +               WARN_ON(1);
> +       }
> +}
>
> It would be better to split the device tree related changes and the
> driver buffer management related changes in separate patches.

Ok.

> +
> +static void __init exynos4_reserve(void)
> +{
> +       s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
> +}
> +
>  DT_MACHINE_START(EXYNOS4210_DT, "Samsung Exynos4 (Flattened Device Tree)")
>         /* Maintainer: Thomas Abraham <thomas.abraham@linaro.org> */
>         .init_irq       = exynos4_init_irq,
> @@ -93,4 +114,5 @@ DT_MACHINE_START(EXYNOS4210_DT, "Samsung Exynos4 (Flattened Device Tree)")
>         .timer          = &exynos4_timer,
>         .dt_compat      = exynos4210_dt_compat,
>         .restart        = exynos4_restart,
> +       .reserve        = exynos4_reserve,
>  MACHINE_END
> diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c b/arch/arm/mach-exynos/mach-exynos5-dt.c
> index ef770bc..898d2de 100644
> --- a/arch/arm/mach-exynos/mach-exynos5-dt.c
> +++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
> @@ -11,6 +11,7 @@
>
>  #include <linux/of_platform.h>
>  #include <linux/serial_core.h>
> +#include <linux/memblock.h>
>
>  #include <asm/mach/arch.h>
>  #include <asm/hardware/gic.h>
> @@ -56,6 +57,7 @@ static const struct of_dev_auxdata exynos5250_auxdata_lookup[] __initconst = {
>         OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_PDMA0, "dma-pl330.0", NULL),
>         OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_PDMA1, "dma-pl330.1", NULL),
>         OF_DEV_AUXDATA("arm,pl330", EXYNOS5_PA_MDMA1, "dma-pl330.2", NULL),
> +       OF_DEV_AUXDATA("samsung,s5p-mfc-v6", 0x11000000, "s5p-mfc-v6", NULL),
>         {},
>  };
>
> @@ -76,6 +78,25 @@ static char const *exynos5250_dt_compat[] __initdata = {
>         NULL
>  };
>
> +static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
> +               phys_addr_t lbase, unsigned int lsize) {
> +
> +       if (memblock_remove(lbase, lsize)) {
> +               pr_err("Failed to reserve bank1 memory for MFC device
");
> +               WARN_ON(1);
> +       }
> +
> +       if (memblock_remove(rbase, rsize)) {
> +               pr_err("Failed to reserve bank2 memory for MFC device
");
> +               WARN_ON(1);
> +       }
> +}
> +
> +static void __init exynos5_reserve(void)
> +{
> +       s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
> +}
> +
>  DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5 (Flattened Device Tree)")
>         /* Maintainer: Kukjin Kim <kgene.kim@samsung.com> */
>         .init_irq       = exynos5_init_irq,
> @@ -86,4 +107,5 @@ DT_MACHINE_START(EXYNOS5_DT, "SAMSUNG EXYNOS5 (Flattened Device Tree)")
>         .timer          = &exynos4_timer,
>         .dt_compat      = exynos5250_dt_compat,
>         .restart        = exynos5_restart,
> +       .reserve        = exynos5_reserve,
>  MACHINE_END
> --
> 1.7.0.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
<p>&nbsp;</p><p>&nbsp;</p>

Re: [PATCH] ARM: EXYNOS: Add MFC device tree support

[Karol Lewandowski]

On 08/16/2012 08:42 PM, Thomas Abraham wrote:
> On 16 August 2012 18:01, Arun Kumar K <arun.kk ... @public.gmane.org> wrote:

>> +  - interrupts : MFC interupt number to the CPU.
>> +
>> +  - samsung,mfc-r : Base address of the first memory bank used by MFC
>> +                   for DMA contiguous memory allocation.
>> +
>> +  - samsung,mfc-r-size : Size of the first memory bank.
> 
> It is not allowed to pass buffer base address and size from device
> tree. Device tree node should describe only the MFC controller
> hardware. Any memory management related information should be handled
> outside of device tree. This helps the bindings to be reusable across
> multiple operating systems.

The question is where elsewhere this should be described as this is strictly
board-dependent option (number and size of RAM banks are important here).

I agree that base addresses are bad, but I'm not aware of any functionality
that would allow driver (actually, its platform dependent part in
exynosN_reserve() function) to enumerate available memory banks and grab
memory chunks from two distinct banks.

My (lack of) knowledge ARM might be to blame here but I simply don't know
how to achieve this. Any suggestions?


On 08/16/2012 09:31 PM, Arun Kumar K wrote:

> +static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
> +		phys_addr_t lbase, unsigned int lsize) {
> +
> +	if (memblock_remove(lbase, lsize)) {
> +		pr_err("Failed to reserve bank1 memory for MFC device\n");
> +		WARN_ON(1);
> +	}
> +
> +	if (memblock_remove(rbase, rsize)) {
> +		pr_err("Failed to reserve bank2 memory for MFC device\n");
> +		WARN_ON(1);
> +	}
> +}


non-static function with the same name is already defined in
arch/arm/plat-samsung/s5p-dev-mfc.c. Please don't duplicate it,
especially that you seem to be trying to do that twice!


> diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c b/arch/arm/mach-exynos/mach-exynos5-dt.c

> index ef770bc..898d2de 100644
> --- a/arch/arm/mach-exynos/mach-exynos5-dt.c
> +++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
...
> +static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
> +		phys_addr_t lbase, unsigned int lsize) {
> +
> +	if (memblock_remove(lbase, lsize)) {
> +		pr_err("Failed to reserve bank1 memory for MFC device\n");
> +		WARN_ON(1);
> +	}
> +
> +	if (memblock_remove(rbase, rsize)) {
> +		pr_err("Failed to reserve bank2 memory for MFC device\n");
> +		WARN_ON(1);
> +	}
> +}


See above.

> +
> +static void __init exynos5_reserve(void)
> +{
> +	s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);


I think it would make sense to make this memory reservation dependent
on "mfc*" node being present in DTS.  It's to early to use of_* functions
(because tree is not populated at this stage) but fdt_* family of functions
work just fine.

Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

RE: [PATCH] ARM: EXYNOS: Add MFC device tree support

[Kukjin Kim]

Arun Kumar K wrote:
> 
> Hi Thomas,
> Thank you for the comments.
> Please find my response inline.
> 
> ------- Original Message -------
> Sender : Thomas Abraham<thomas.abraham@linaro.org>
> Date   : Aug 16, 2012 17:12 (GMT+05:30)
> Title  : Re: [PATCH] ARM: EXYNOS: Add MFC device tree support
> 
> On 16 August 2012 18:01, Arun Kumar K <arun.kk@samsung.com> wrote:
> > From: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> >
> > This patch adds device tree entry for MFC in the Exynos
> > machines. Exynos4 SoCs support MFC v5 version and Exynos5 has
> > MFC v6.x version. So making the required changes in the clock

Since v6.1 is not used anywhere so just MFC v6 should be ok.

> > files and adds MFC to the DT device list.
> >
> > Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
> > Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> > ---
> >  .../devicetree/bindings/media/s5p-mfc.txt          |   24
> ++++++++++++++++++++
> >  arch/arm/boot/dts/exynos4210.dtsi                  |   10 ++++++++
> >  arch/arm/boot/dts/exynos5250.dtsi                  |   10 ++++++++
> >  arch/arm/mach-exynos/clock-exynos5.c               |    2 +-
> >  arch/arm/mach-exynos/mach-exynos4-dt.c             |   22
> ++++++++++++++++++
> >  arch/arm/mach-exynos/mach-exynos5-dt.c             |   22
> ++++++++++++++++++
> >  6 files changed, 89 insertions(+), 1 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/media/s5p-mfc.txt
> >
> > diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt
> b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> > new file mode 100644
> > index 0000000..b9bd266
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
> > @@ -0,0 +1,24 @@
> > +* Samsung Multi Format Codec (MFC)
> > +
> > +Mult Format Codec (MFC) is the IP present in Samsung SoCs which
> > +supports high resolution decoding and encoding functionalities.
> >
> > In addition to this, specifying that mfc is used for video
> > encode/decode would be informative.
> 
> Ok. I will make it more descriptive.
> 
> > +
> > +Required properties:
> > +  - compatible : value should be either one among the following
> > +       (a) "samsung,s5p-mfc-v5" for MFC v5 present in Exynos4 SoCs
> > +       (b) "samsung,s5p-mfc-v6" for MFC v6.x present in Exynos5 SoCs
> >
> > "s5p" should be dropped from the compatible values. For example, it
> > should be "samsung,mfc-v5", which is sufficient to identify the
> > version of the mfc controller.
> 
> Ok will remove s5p.
> 
Yeah, I agree, just 'mfc-vX' is enough.

> > +
> > +  - reg : Physical base address of the IP registers and length of
> memory
> > +         mapped region.
> > +
> > +  - interrupts : MFC interupt number to the CPU.
> > +
> > +  - samsung,mfc-r : Base address of the first memory bank used by MFC
> > +                   for DMA contiguous memory allocation.
> > +
> > +  - samsung,mfc-r-size : Size of the first memory bank.
> >
> > It is not allowed to pass buffer base address and size from device
> > tree. Device tree node should describe only the MFC controller
> > hardware. Any memory management related information should be handled
> > outside of device tree. This helps the bindings to be reusable across
> > multiple operating systems.
> 
> The mfc-l and mfc-r base address and size is board specific info which has
> to
> be passed to the driver. This is used for DMA contiguous allocation by
> driver and this value
> can change on a different board.
> So in that case, i will pass it as platform data to the driver from mach-
> exynos5-dt.c file.
> I hope that would be ok.
> 
I don't think so. The mach-exynos5-dt is for all EXYNOS5 SoCs not
platform_data. As I know, the addresses and sizes for buffer passed via
platform data before, so it can be passed via device tree for board(.dtsi)?
not SoC. In addition, it depends on board.

> > +
> > +  - samsung,mfc-l : Base address of the second memory bank used by MFC
> > +                   for DMA contiguous memory allocation.
> > +
> > +  - samsung,mfc-l-size : Size of the second memory bank.
> >
> > Same comment as above. And the bindings documentation is usually
> > included in the same patch that adds device tree support for the
> > driver.
> 
> Ok
> 
> > diff --git a/arch/arm/boot/dts/exynos4210.dtsi
> b/arch/arm/boot/dts/exynos4210.dtsi
> > index 02891fe..b5ee43d 100644
> > --- a/arch/arm/boot/dts/exynos4210.dtsi
> > +++ b/arch/arm/boot/dts/exynos4210.dtsi
> > @@ -56,6 +56,16 @@
> >                 interrupts = <0 43 0>;
> >         };
> >
> > +       mfc {
> > +               compatible = "samsung,s5p-mfc";

Maybe
+	compatible = "samsung,mfc-v5"; ?

> > +               reg = <0x13400000 0x10000>;
> > +               interrupts = <0 94 0>;
> > +               samsung,mfc-r = <0x43000000>;
> > +               samsung,mfc-r-size = <8388608>;
> > +               samsung,mfc-l = <0x51000000>;
> > +               samsung,mfc-l-size = <8388608>;

As I commented, the size depends on each board not SoC. So should be removed
here.

[...]

Thanks.

Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.

RE: [PATCH] ARM: EXYNOS: Add MFC device tree support

[Arun Kumar K]

Hi Kgene,
Thank you for the review comments.
Please find my response inline.

On Thu, Aug 23, 2012 at 1:46 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Arun Kumar K wrote:
>>
>> Hi Thomas,
>> Thank you for the comments.
>> Please find my response inline.
>>
>> ------- Original Message -------
>> Sender : Thomas Abraham<thomas.abraham@linaro.org>
>> Date   : Aug 16, 2012 17:12 (GMT+05:30)
>> Title  : Re: [PATCH] ARM: EXYNOS: Add MFC device tree support
>>
>> On 16 August 2012 18:01, Arun Kumar K <arun.kk@samsung.com> wrote:
>> > From: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
>> >
>> > This patch adds device tree entry for MFC in the Exynos
>> > machines. Exynos4 SoCs support MFC v5 version and Exynos5 has
>> > MFC v6.x version. So making the required changes in the clock
>
> Since v6.1 is not used anywhere so just MFC v6 should be ok.
>

Ok.

>> > files and adds MFC to the DT device list.
>> >
>> > Signed-off-by: Naveen Krishna Chatradhi <ch.naveen@samsung.com>
>> > Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
>> > ---
>> >  .../devicetree/bindings/media/s5p-mfc.txt          |   24
>> ++++++++++++++++++++
>> >  arch/arm/boot/dts/exynos4210.dtsi                  |   10 ++++++++
>> >  arch/arm/boot/dts/exynos5250.dtsi                  |   10 ++++++++
>> >  arch/arm/mach-exynos/clock-exynos5.c               |    2 +-
>> >  arch/arm/mach-exynos/mach-exynos4-dt.c             |   22
>> ++++++++++++++++++
>> >  arch/arm/mach-exynos/mach-exynos5-dt.c             |   22
>> ++++++++++++++++++
>> >  6 files changed, 89 insertions(+), 1 deletions(-)
>> >  create mode 100644 Documentation/devicetree/bindings/media/s5p-mfc.txt
>> >
>> > diff --git a/Documentation/devicetree/bindings/media/s5p-mfc.txt
>> b/Documentation/devicetree/bindings/media/s5p-mfc.txt
>> > new file mode 100644
>> > index 0000000..b9bd266
>> > --- /dev/null
>> > +++ b/Documentation/devicetree/bindings/media/s5p-mfc.txt
>> > @@ -0,0 +1,24 @@
>> > +* Samsung Multi Format Codec (MFC)
>> > +
>> > +Mult Format Codec (MFC) is the IP present in Samsung SoCs which
>> > +supports high resolution decoding and encoding functionalities.
>> >
>> > In addition to this, specifying that mfc is used for video
>> > encode/decode would be informative.
>>
>> Ok. I will make it more descriptive.
>>
>> > +
>> > +Required properties:
>> > +  - compatible : value should be either one among the following
>> > +       (a) "samsung,s5p-mfc-v5" for MFC v5 present in Exynos4 SoCs
>> > +       (b) "samsung,s5p-mfc-v6" for MFC v6.x present in Exynos5 SoCs
>> >
>> > "s5p" should be dropped from the compatible values. For example, it
>> > should be "samsung,mfc-v5", which is sufficient to identify the
>> > version of the mfc controller.
>>
>> Ok will remove s5p.
>>
> Yeah, I agree, just 'mfc-vX' is enough.
>

Ok. Will change it.

>> > +
>> > +  - reg : Physical base address of the IP registers and length of
>> memory
>> > +         mapped region.
>> > +
>> > +  - interrupts : MFC interupt number to the CPU.
>> > +
>> > +  - samsung,mfc-r : Base address of the first memory bank used by MFC
>> > +                   for DMA contiguous memory allocation.
>> > +
>> > +  - samsung,mfc-r-size : Size of the first memory bank.
>> >
>> > It is not allowed to pass buffer base address and size from device
>> > tree. Device tree node should describe only the MFC controller
>> > hardware. Any memory management related information should be handled
>> > outside of device tree. This helps the bindings to be reusable across
>> > multiple operating systems.
>>
>> The mfc-l and mfc-r base address and size is board specific info which has
>> to
>> be passed to the driver. This is used for DMA contiguous allocation by
>> driver and this value
>> can change on a different board.
>> So in that case, i will pass it as platform data to the driver from mach-
>> exynos5-dt.c file.
>> I hope that would be ok.
>>
> I don't think so. The mach-exynos5-dt is for all EXYNOS5 SoCs not
> platform_data. As I know, the addresses and sizes for buffer passed via
> platform data before, so it can be passed via device tree for board(.dtsi)?
> not SoC. In addition, it depends on board.
>

Ok. So as suggested the best option would be to move the mfc-l and r 
base address and size information to board dts file 
(exynos5250-smdk5250.dts) from exynos5250.dtsi. Hope this would be
good.


>> > +
>> > +  - samsung,mfc-l : Base address of the second memory bank used by MFC
>> > +                   for DMA contiguous memory allocation.
>> > +
>> > +  - samsung,mfc-l-size : Size of the second memory bank.
>> >
>> > Same comment as above. And the bindings documentation is usually
>> > included in the same patch that adds device tree support for the
>> > driver.
>>
>> Ok
>>
>> > diff --git a/arch/arm/boot/dts/exynos4210.dtsi
>> b/arch/arm/boot/dts/exynos4210.dtsi
>> > index 02891fe..b5ee43d 100644
>> > --- a/arch/arm/boot/dts/exynos4210.dtsi
>> > +++ b/arch/arm/boot/dts/exynos4210.dtsi
>> > @@ -56,6 +56,16 @@
>> >                 interrupts = <0 43 0>;
>> >         };
>> >
>> > +       mfc {
>> > +               compatible = "samsung,s5p-mfc";
>
> Maybe
> +       compatible = "samsung,mfc-v5"; ?
>
>> > +               reg = <0x13400000 0x10000>;
>> > +               interrupts = <0 94 0>;
>> > +               samsung,mfc-r = <0x43000000>;
>> > +               samsung,mfc-r-size = <8388608>;
>> > +               samsung,mfc-l = <0x51000000>;
>> > +               samsung,mfc-l-size = <8388608>;
>
> As I commented, the size depends on each board not SoC. So should be removed
> here.
>

Ok

> [...]
>

Thanks and regards
Arun

Re: [PATCH] ARM: EXYNOS: Add MFC device tree support

[Arun Kumar K]

Hi Karol,
Thanks for your comments. 
Please find my response inline.

On Mon, Aug 20, 2012 at 11:47 AM, Karol Lewandowski <k.lewandowsk@samsung.com> wrote:
> On 08/16/2012 08:42 PM, Thomas Abraham wrote:
>> On 16 August 2012 18:01, Arun Kumar K <arun.kk ... @public.gmane.org> wrote:
>
>>> +  - interrupts : MFC interupt number to the CPU.
>>> +
>>> +  - samsung,mfc-r : Base address of the first memory bank used by MFC
>>> +                   for DMA contiguous memory allocation.
>>> +
>>> +  - samsung,mfc-r-size : Size of the first memory bank.
>>
>> It is not allowed to pass buffer base address and size from device
>> tree. Device tree node should describe only the MFC controller
>> hardware. Any memory management related information should be handled
>> outside of device tree. This helps the bindings to be reusable across
>> multiple operating systems.
>
> The question is where elsewhere this should be described as this is strictly
> board-dependent option (number and size of RAM banks are important here).
>
> I agree that base addresses are bad, but I'm not aware of any functionality
> that would allow driver (actually, its platform dependent part in
> exynosN_reserve() function) to enumerate available memory banks and grab
> memory chunks from two distinct banks.
>
> My (lack of) knowledge ARM might be to blame here but I simply don't know
> how to achieve this. Any suggestions?
>

As suggested by kgene, I will pass it from the board specific dts file.

>
> On 08/16/2012 09:31 PM, Arun Kumar K wrote:
>
>> +static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
>> +             phys_addr_t lbase, unsigned int lsize) {
>> +
>> +     if (memblock_remove(lbase, lsize)) {
>> +             pr_err("Failed to reserve bank1 memory for MFC device\n");
>> +             WARN_ON(1);
>> +     }
>> +
>> +     if (memblock_remove(rbase, rsize)) {
>> +             pr_err("Failed to reserve bank2 memory for MFC device\n");
>> +             WARN_ON(1);
>> +     }
>> +}
>
>
> non-static function with the same name is already defined in
> arch/arm/plat-samsung/s5p-dev-mfc.c. Please don't duplicate it,
> especially that you seem to be trying to do that twice!
>

Ok, I will use the existing function.

>
>> diff --git a/arch/arm/mach-exynos/mach-exynos5-dt.c b/arch/arm/mach-exynos/mach-exynos5-dt.c
>
>> index ef770bc..898d2de 100644
>> --- a/arch/arm/mach-exynos/mach-exynos5-dt.c
>> +++ b/arch/arm/mach-exynos/mach-exynos5-dt.c
> ...
>> +static void s5p_mfc_reserve_mem(phys_addr_t rbase, unsigned int rsize,
>> +             phys_addr_t lbase, unsigned int lsize) {
>> +
>> +     if (memblock_remove(lbase, lsize)) {
>> +             pr_err("Failed to reserve bank1 memory for MFC device\n");
>> +             WARN_ON(1);
>> +     }
>> +
>> +     if (memblock_remove(rbase, rsize)) {
>> +             pr_err("Failed to reserve bank2 memory for MFC device\n");
>> +             WARN_ON(1);
>> +     }
>> +}
>
>
> See above.
>
>> +
>> +static void __init exynos5_reserve(void)
>> +{
>> +     s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
>
>
> I think it would make sense to make this memory reservation dependent
> on "mfc*" node being present in DTS.  It's to early to use of_* functions
> (because tree is not populated at this stage) but fdt_* family of functions
> work just fine.
>

As I can see the fdt_* functions are not used in any of the ARM based SoC
init codes. Though I can see some references in powerpc.
The implementation and includes are present in arch/arm/boot/compressed/
which I think cannot be used directly in mach-exynos unless we make some
comon makefile changes. 
Please clarify whether its ok to use fdt_* functions to parse the dts in 
exynos machine init or please point me to some sample implementations
which I can refer to.

Regards
Arun

Regards
Arun

Re: [PATCH] ARM: EXYNOS: Add MFC device tree support

[Karol Lewandowski]

On 08/28/2012 07:08 PM, Arun Kumar K wrote:

> Hi Karol,
> Thanks for your comments. 
> Please find my response inline.

Hi... and sorry for so much delayed response.

>>> +
>>> +static void __init exynos5_reserve(void)
>>> +{
>>> +     s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
>>
>>
>> I think it would make sense to make this memory reservation dependent
>> on "mfc*" node being present in DTS.  It's to early to use of_* functions
>> (because tree is not populated at this stage) but fdt_* family of functions
>> work just fine.
>>
> 
> As I can see the fdt_* functions are not used in any of the ARM based SoC
> init codes. Though I can see some references in powerpc.
> The implementation and includes are present in arch/arm/boot/compressed/
> which I think cannot be used directly in mach-exynos unless we make some
> comon makefile changes. 


It looks like I was writing from memory, and I actually mixed things 
up.  To be clear this time - we can't use regular device tree handling
functions in reserve() as it's too early.  Namely, flattened device tree
is not yet converted to kernel's-natural representation.  However, 
I think we can scan fdt just fine.  To do so one just needs to use
functions declared here

  #include <linux/of_fdt.h>

Actual architecture-independent code is in drivers/of/fdt.c.  This
provides of_fdt_ family of functions. Please see below for example.


> Please clarify whether its ok to use fdt_* functions to parse the dts in 
> exynos machine init or please point me to some sample implementations
> which I can refer to.


It should be ok to use anything that works on flattened device tree
rather than its uncompressed version.  I've experimented a bit and
something like this worked for me just fine (it was around 3.3-kernel
timeframe, but I don't think that fdt api has changed):

[mach-exynos4-dt.c]

#include <linux/of_fdt.h>

int fdt_find_compat(unsigned long node, const char *uname, int depth, void *data)
{
        if (of_flat_dt_is_compatible(node, (char *)data))
                return 1;

        return 0;
}

static void __init exynos4210_dt_reserve(void)
{
        /* Reserve memory for MFC only if it's available */
        if (of_scan_flat_dt(fdt_find_compat, "samsung,s5pv210-mfc")) {
                printk(KERN_NOTICE "exynos4-dt: mfc device node found - setting up memory area for dma\n");
                s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
        }
}

[.dts]

 codec@some-addr {
      compatible = "samsung,s5pv210-mfc";
 };


So, in above code fragment I just check if mfc was defined in dts.
This could probably stay as it is.

Then I allocate _predefined_ region - and this part should be fixed.

If you have nodes like "mfc-r-size"/offset, then you could just get
this information directly from (f)dt rather than hardcoding it in the code.
Precisely, after we find compatible node we could do something like
following (untested):

unsigned long lsize, loff, rsize, roff len;
__be32 *prop;

prop = of_get_flat_dt_prop(node, "samsung,mfc-l-size", &len);
if (!prop)
   return;
lsize = of_read_ulong(prop, len/4);
...

Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

Re: [PATCH] ARM: EXYNOS: Add MFC device tree support

[Arun Kumar K]

Hi Karol,
Thank you very much for the detailed explanation.
Its indeed very well explained and seems like a great approach 
to remove the hard codings.
I will go ahead with this implementation and post the updated patch.

Regards
Arun

On Wed, Sep 5, 2012 at 8:12 AM, Karol Lewandowski <k.lewandowsk@samsung.com> wrote:
> On 08/28/2012 07:08 PM, Arun Kumar K wrote:
>
>> Hi Karol,
>> Thanks for your comments.
>> Please find my response inline.
>
> Hi... and sorry for so much delayed response.
>
>>>> +
>>>> +static void __init exynos5_reserve(void)
>>>> +{
>>>> +     s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
>>>
>>>
>>> I think it would make sense to make this memory reservation dependent
>>> on "mfc*" node being present in DTS.  It's to early to use of_* functions
>>> (because tree is not populated at this stage) but fdt_* family of functions
>>> work just fine.
>>>
>>
>> As I can see the fdt_* functions are not used in any of the ARM based SoC
>> init codes. Though I can see some references in powerpc.
>> The implementation and includes are present in arch/arm/boot/compressed/
>> which I think cannot be used directly in mach-exynos unless we make some
>> comon makefile changes.
>
>
> It looks like I was writing from memory, and I actually mixed things
> up.  To be clear this time - we can't use regular device tree handling
> functions in reserve() as it's too early.  Namely, flattened device tree
> is not yet converted to kernel's-natural representation.  However,
> I think we can scan fdt just fine.  To do so one just needs to use
> functions declared here
>
>   #include <linux/of_fdt.h>
>
> Actual architecture-independent code is in drivers/of/fdt.c.  This
> provides of_fdt_ family of functions. Please see below for example.
>
>
>> Please clarify whether its ok to use fdt_* functions to parse the dts in
>> exynos machine init or please point me to some sample implementations
>> which I can refer to.
>
>
> It should be ok to use anything that works on flattened device tree
> rather than its uncompressed version.  I've experimented a bit and
> something like this worked for me just fine (it was around 3.3-kernel
> timeframe, but I don't think that fdt api has changed):
>
> [mach-exynos4-dt.c]
>
> #include <linux/of_fdt.h>
>
> int fdt_find_compat(unsigned long node, const char *uname, int depth, void *data)
> {
>         if (of_flat_dt_is_compatible(node, (char *)data))
>                 return 1;
>
>         return 0;
> }
>
> static void __init exynos4210_dt_reserve(void)
> {
>         /* Reserve memory for MFC only if it's available */
>         if (of_scan_flat_dt(fdt_find_compat, "samsung,s5pv210-mfc")) {
>                 printk(KERN_NOTICE "exynos4-dt: mfc device node found - setting up memory area for dma\n");
>                 s5p_mfc_reserve_mem(0x43000000, 8 << 20, 0x51000000, 8 << 20);
>         }
> }
>
> [.dts]
>
>  codec@some-addr {
>       compatible = "samsung,s5pv210-mfc";
>  };
>
>
> So, in above code fragment I just check if mfc was defined in dts.
> This could probably stay as it is.
>
> Then I allocate _predefined_ region - and this part should be fixed.
>
> If you have nodes like "mfc-r-size"/offset, then you could just get
> this information directly from (f)dt rather than hardcoding it in the code.
> Precisely, after we find compatible node we could do something like
> following (untested):
>
> unsigned long lsize, loff, rsize, roff len;
> __be32 *prop;
>
> prop = of_get_flat_dt_prop(node, "samsung,mfc-l-size", &len);
> if (!prop)
>    return;
> lsize = of_read_ulong(prop, len/4);
> ...
>
> Regards,
> --
> Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html