Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S968484AbdIZMCu (ORCPT ); Tue, 26 Sep 2017 08:02:50 -0400 Received: from mail-lf0-f65.google.com ([209.85.215.65]:38425 "EHLO mail-lf0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934961AbdIZMCs (ORCPT ); Tue, 26 Sep 2017 08:02:48 -0400 X-Google-Smtp-Source: AOwi7QBidlTDK4/l0OBN2BgqIigfCOufH/g/3wobfpUU+Tvwt4Y1aaWj0//Y4l8JjYZn7ikpj1vrYQ== Subject: Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver To: Stephen Warren Cc: Thierry Reding , Jonathan Hunter , Greg Kroah-Hartman , Rob Herring , linux-tegra@vger.kernel.org, devel@driverdev.osuosl.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org References: <5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com> <38daee0d-0595-a7e4-69bb-5a2ddbd832b5@wwwdotorg.org> <52f5c05f-7060-4d0f-355f-6da372c9ded0@wwwdotorg.org> From: Dmitry Osipenko Message-ID: <113675da-6b85-ef2f-03a9-84e5cb93c31b@gmail.com> Date: Tue, 26 Sep 2017 15:02:44 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <52f5c05f-7060-4d0f-355f-6da372c9ded0@wwwdotorg.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1959 Lines: 48 On 26.09.2017 08:11, Stephen Warren wrote: > On 09/25/2017 05:45 PM, Dmitry Osipenko wrote: >> On 26.09.2017 02:01, Stephen Warren wrote: >>> On 09/25/2017 04:15 PM, Dmitry Osipenko wrote: >>>> Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of >>>> video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports >>>> decoding of CAVLC H.264 only. >>> >>> Note: I don't know anything much about video decoding on Tegra (just NV desktop >>> GPUs, and that was a while ago), but I had a couple small comments on the DT >>> binding: >>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt >>>> b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt >>> >>>> +NVIDIA Tegra Video Decoder Engine >>>> + >>>> +Required properties: >>>> +- compatible : "nvidia,tegra20-vde" >>>> +- reg : Must contain 2 register ranges: registers and IRAM area. >>>> +- reg-names : Must include the following entries: >>>> + - regs >>>> + - iram >>> >>> I think the IRAM region needs more explanation: What is the region used for and >>> by what? Can it be moved, and if so does the move need to be co-ordinated with >>> any other piece of SW? >> >> IRAM region is used by Video Decoder HW for internal use and some of decoding >> parameters are supplied via IRAM, like frames order list. AFAIK IRAM addresses >> are hardwired in HW and aren't movable, it is not 100% but I'm pretty sure. >> Should it be explained in the binding? > > I think this should be briefly mentioned, yes. Otherwise at least people > who don't know the VDE HW well (like me) will wonder why on earth VDE > interacts with IRAM at all. I would have assumed all parameters were > supplied via registers or via descriptors in DRAM. > > Thanks. > I also forgot to mention that VDE scrubs that IRAM region on HW reset. So yeah, it's definitely a part of HW definition. I'll add a brief explanation to the binding. -- Dmitry