Message-ID: <b1b2a510-e0cb-5d8f-9f44-48c9dd747d4d@ti.com>
Date:	Thu, 11 Aug 2016 11:23:30 -0500
From:	Suman Anna <s-anna@...com>
To:	Lee Jones <lee.jones@...aro.org>
CC:	Bjorn Andersson <bjorn.andersson@...aro.org>,
	Tony Lindgren <tony@...mide.com>,
	"ohad@...ery.com" <ohad@...ery.com>,
	"kernel@...inux.com" <kernel@...inux.com>,
	"linux-remoteproc@...r.kernel.org" <linux-remoteproc@...r.kernel.org>,
	"patrice.chotard@...com" <patrice.chotard@...com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"ludovic.barre@...com" <ludovic.barre@...com>,
	"ssantosh@...nel.org" <ssantosh@...nel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Dave Gerlach <d-gerlach@...com>
Subject: Re: [PATCH 1/2] remoteproc: core: Add rproc OF look-up functions

On 08/11/2016 11:04 AM, Suman Anna wrote:
> Hi Lee,
> 
> On 08/11/2016 02:31 AM, Lee Jones wrote:
>> On Wed, 10 Aug 2016, Suman Anna wrote:
>>
>>> On 08/10/2016 04:19 PM, Bjorn Andersson wrote:
>>>> On Wed 10 Aug 14:04 PDT 2016, Suman Anna wrote:
>>>>
>>>>> On 08/10/2016 03:40 PM, Bjorn Andersson wrote:
>>>>>> On Wed 10 Aug 12:37 PDT 2016, Suman Anna wrote:
>>>>>>
>>>>>>> Hi Lee, Bjorn,
>>>>>>>
>>>>>>> On 08/10/2016 12:40 PM, Bjorn Andersson wrote:
>>>>>>>> On Tue 19 Jul 08:49 PDT 2016, Lee Jones wrote:
>>>>>>>>
>>>>>>>>> - of_rproc_by_index(): look-up and obtain a reference to a rproc
>>>>>>>>>                        using the DT phandle "rprocs" and a index.
>>>>>>>>>
>>>>>>>>> - of_rproc_by_name():  lookup and obtain a reference to a rproc
>>>>>>>>>                        using the DT phandle "rprocs" and "rproc-names".
>>>>>>>>>
>>>>>>>>> Signed-off-by: Ludovic Barre <ludovic.barre@...com>
>>>>>>>>> Signed-off-by: Lee Jones <lee.jones@...aro.org>
>>>>>>>>> ---
>>>>>>>>
>>>>>>>> I'm happy with this, so I whipped up a binding document describing our
>>>>>>>> two new properties. Waiting for an opinion on that before I merge this.
>>>>>>>
>>>>>>> Yeah, I like the two new API too, I have just about run into the need
>>>>>>> for the same on my product trees. We can probably make it less
>>>>>>> complicated than what the current series is. The wkup_m3_ipc usage was
>>>>>>> before there was any standardization on the property names, so we went
>>>>>>> with a ti, prefixed property specific to the wkup_m3_ipc node and a
>>>>>>> general API that is agnostic of property name. We don't have all the
>>>>>>> pieces for PM on AM335x/AM437x SoCs on upstream kernel yet, so we should
>>>>>>> be able to switch over to using the new property as we cannot achieve
>>>>>>> the desired functionality even though we can boot the wkup_m3.
>>>>>>>
>>>>>>
>>>>>> Glad to hear you're onboard with dropping the old property name, even if
>>>>>> it's later.
>>>>>>
>>>>>>> Here's what I propose:
>>>>>>> 1. Let's not burden the new of_get_rproc_by_index() API with having to
>>>>>>> fall-back and look for ti,rprocs. The rproc_get_by_phandle() core logic
>>>>>>> of looking up against the rproc list is self-contained, and the API
>>>>>>> usage is limited to just the wkup_m3_ipc driver at the moment.
>>>>>>> 2. Keep the rproc_get_by_phandle API as is but mark it as deprecated.
>>>>>>> IMHO, the rename of this API to of_get_rproc_by_phandle() in Patch 2 but
>>>>>>> using a device node pointer as input argument doesn't add any value.
>>>>>>> 3. Provide a fallback in wkup_m3_ipc driver to look for both rprocs and
>>>>>>> ti,rproc property, while switching over the node to use rprocs property.
>>>>>>> 4. We can get rid of the rproc_get_by_phandle() API after the
>>>>>>> wkup_m3_ipc transition is done to use of_get_rproc_by_index() API.
>>>>>>>
>>>>>>
>>>>>> I don't fancy the idea of leaving the kernel builds with a deprecation
>>>>>> warning for some time and I don't feel the cost of carrying those 2
>>>>>> extra statements is a bigger issue than keeping a deprecated API around.
>>>>>> And it will be less than the dance we have to do in wkup_m3_ipc.
>>>>>>
>>>>>> So I think that we should merge these patches and if it can be concluded
>>>>>> that we don't need backwards compatibility with the old DT property we
>>>>>> can drop the inner conditional in the API.
>>>>>
>>>>> Yeah, I am fine with dropping the inner ti,rproc processing in the new
>>>>> of_rproc_get_by_index() API and keeping it clean. What's not clear to me
>>>>> is why we would still need a get_by_phandle API alongside the two new
>>>>> API once the wkup_m3_ipc is converted to the new API?  Or is it just to
>>>>> clean up the consumer interface? If latter, I will fixup the wkup_m3_ipc
>>>>> driver without the need for remoteproc core changes, and switch over to
>>>>> the new API.
>>>>>
>>>>
>>>> of_get_rproc_by_phandle() is just a convenience for not having to get by
>>>> index 0, as most drivers is only having one rproc.
>>>
>>> OK, then better to name this as simply of_rproc_get(), the _by_phandle()
>>> does not match up with the other two of_get_rproc_xxx API.
>>
>> I'm not opposed to changing the call to of_rproc_get().  

Also, since this is gonna be a simple helper, can we make this a static
inline and move to the remoteproc.h header file.


However, I'm
>> not sure what you mean about it not matching the other OF functions.
> 
> The _by_name and _by_index API take in a name and index arguments, and
> the rproc_get_by_phandle took in a phandle argument, the modified name
> in patch 2 retains the by_phandle but without any corresponding
> argument. That's what I meant by mismatch.
> 
>> The nomenclature should be the same of_get_rproc_*(), no?
>>
>>> Suggest also a rename from of_get_rproc_xxx to of_rproc_get_xxx like in
>>> other subsystems.
>>
>> This suggestion does the opposite and does not fit in with the
>> majority of the of_* calls scattered around in other subsystems:
>>
>> of_get_drm
>> of_get_coresight
>> of_get_fb
>> of_get_i2c
>> of_get_regulator
>> of_get_gpio
>> of_get_mac
>> of_get_display
>> of_get_videomode
>>
>> Vs
>>
>> of_irq_get
>> of_reset_get
>> of_graph_get
>> of_msi_get
>> of_usb_get
>> of_genpd_get
>>
>> These guys have both oddly.
>>
>> of_get_dma, of_dma_get
>> of_get_pci, of_pci_get
>> of_get_phy, of_phy_get
> 
> Hmm, looks like there are all sorts of combinations with some like
> <subsys>_of_get_xxx. I did search using get_name and get_index, and
> nothing popped up differently. The ones that typical rproc drivers deal
> with are clk, reset, irq which follow the latter convention. Given that
> the previous API within remoteproc used to be rproc_get,
> rproc_get_by_name (these were dropped sometime back) and
> rproc_get_by_phandle, I still like of_rproc_get_by_index,
> of_rproc_get_by_name, of_rproc_get and then the counter part is the
> regular rproc_put().
> 
> This patch also needs one correction, the inner loop string check should
> be ti,rproc and not ti,rprocs.
> 
> regards
> Suman
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Powered by blists - more mailing lists