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

[Arun Kumar K <arun.kk@samsung.com>]

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