[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 at 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 at samsung.com> wrote:
> > From: Naveen Krishna Chatradhi <ch.naveen at 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 at samsung.com>
> > Signed-off-by: Arun Kumar K <arun.kk at 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 at samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.