Date: Mon, 5 Jun 2017 12:26:48 -0500
From: Rob Herring <robh@kernel.org>
To: Suman Anna <s-anna@ti.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
        Ohad Ben-Cohen <ohad@wizery.com>,
        Santosh Shilimkar <ssantosh@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>, linux-remoteproc@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, "Andrew F. Davis" <afd@ti.com>,
        Sam Nelson <sam.nelson@ti.com>
Subject: Re: [PATCH 1/3] Documentation: DT: add Keystone DSP remoteproc
 binding
Message-ID: <20170605172648.6xonnkvbwmd3cehg@rob-hp-laptop>
References: <20170526165317.23164-1-s-anna@ti.com>
 <20170526165317.23164-2-s-anna@ti.com>
 <20170531191231.5qt2lfv73p572gmd@rob-hp-laptop>
 <884f5585-eac2-57b2-a2be-879bf23f2e00@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <884f5585-eac2-57b2-a2be-879bf23f2e00@ti.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3826
Lines: 86

On Wed, May 31, 2017 at 03:05:59PM -0500, Suman Anna wrote:
> Hi Rob,
> 
> On 05/31/2017 02:12 PM, Rob Herring wrote:
> > On Fri, May 26, 2017 at 11:53:15AM -0500, Suman Anna wrote:
> >> Add the device tree bindings document for the Texas Instrument's
> >> Keystone 2 DSP remoteproc devices.
> >>
> >> Signed-off-by: Suman Anna <s-anna@ti.com>
> >> Signed-off-by: Sam Nelson <sam.nelson@ti.com>
> >> ---
> >>  .../bindings/remoteproc/ti,keystone-rproc.txt      | 132 +++++++++++++++++++++
> >>  1 file changed, 132 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/remoteproc/ti,keystone-rproc.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,keystone-rproc.txt b/Documentation/devicetree/bindings/remoteproc/ti,keystone-rproc.txt
> >> new file mode 100644
> >> index 000000000000..f1ba88edd00d
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/remoteproc/ti,keystone-rproc.txt
> >> @@ -0,0 +1,132 @@
> >> +TI Keystone DSP devices
> >> +=======================
> >> +
> >> +Binding status: Unstable - Subject to changes for using multiple memory regions
> > 
> > I don't really see what would be unstable here. memory-region is easily 
> > extended to multiple entries.
> 
> OK will drop this line.
> 
> > 
> >> +
> >> +The TI Keystone 2 family of SoCs usually have one or more (upto 8) TI DSP Core
> >> +sub-systems that are used to offload some of the processor-intensive tasks or
> >> +algorithms, for achieving various system level goals.
> >> +
> >> +These processor sub-systems usually contain additional sub-modules like L1
> >> +and/or L2 caches/SRAMs, an Interrupt Controller, an external memory controller,
> >> +a dedicated local power/sleep controller etc. The DSP processor core in
> >> +Keystone 2 SoCs is usually a TMS320C66x CorePac processor.
> >> +
> >> +DSP Device Node:
> >> +================
> >> +Each DSP Core sub-system is represented as a single DT node. Each node has a
> >> +number of required or optional properties that enable the OS running on the
> >> +host processor (ARM CorePac) to perform the device management of the remote
> >> +processor and to communicate with the remote processor.
> >> +
> >> +Required properties:
> >> +--------------------
> >> +The following are the mandatory properties:
> >> +
> >> +- compatible:		Should be one of the following,
> >> +			    "ti,k2hk-dsp" for DSPs on Keystone 2 66AK2H/K SoCs
> >> +			    "ti,k2l-dsp" for DSPs on Keystone 2 66AK2L SoCs
> >> +			    "ti,k2e-dsp" for DSPs on Keystone 2 66AK2E SoCs
> >> +
> >> +- reg:			Should contain an entry for each value in 'reg-names'.
> >> +			Each entry should have the memory region's start address
> >> +			and the size of the region, the representation matching
> >> +			the parent node's '#address-cells' and '#size-cells' values.
> >> +
> >> +- reg-names:		Should contain strings with the following names, each
> >> +			representing a specific internal memory region, and
> >> +			should be defined in this order,
> >> +			     "l2sram", "l1pram", "l1dram"
> >> +
> >> +- label:		Should contain a string identifying the DSP instance
> >> +			within the SoC. Should be the string "dsp" followed by
> >> +			the instance number. Eg: "dsp0", "dsp1",..."dsp7" etc
> > 
> > Why does a user need to know or care?
> 
> This is used to distinguish the exact DSP instance from others since the
> DT node name is a generic "dsp". One of the uses is to be able to
> construct a firmware name within the driver using this label.

firmware-name doesn't work for you?

It was obvious that you are using this to number instances. Why do you 
care which instance is which? For example, I want dsp0 because it has X 
or runs at XXMHz, etc. If each instance is different (i.e. not a pool of 
DSPs), then you should describe the difference some way.

Rob