[PATCH] soc: ti: wkup_m3_ipc: switch to using remoteproc OF infrastructure
Suman Anna
s-anna at ti.com
Thu Aug 18 14:30:59 PDT 2016
Hi Rob,
On 08/16/2016 09:54 AM, Rob Herring wrote:
> On Fri, Aug 12, 2016 at 11:43 AM, Bjorn Andersson
> <bjorn.andersson at linaro.org> wrote:
>> On Fri 12 Aug 09:00 PDT 2016, Suman Anna wrote:
>>
>>> On 08/12/2016 06:02 AM, Lee Jones wrote:
>>>> On Thu, 11 Aug 2016, Suman Anna wrote:
>>>>
>>>>> The remoteproc framework has been enhanced recently to provide new
>>>>> OF API to retrieve a remoteproc handle by client drivers through a
>>>>> standard 'rprocs' property in client nodes. The wkup_m3_ipc driver
>>>>> has been using a custom 'ti,rproc' property until now, switch this
>>>>> to use this new OF infrastructure. The wkup_m3_ipc driver has been
>>>>> fixed up to provide backward compatibility for older DTBs by
>>>>> replacing the older property with the standard newer property.
>>>>>
>>>>> The wkup_m3_ipc binding has also been updated accordingly.
>>>>>
>>>>> Signed-off-by: Suman Anna <s-anna at ti.com>
>>>>> ---
>>>>> This patch is based on the discussion [1] for introducing new standard
>>>>> OF API in remoteproc core series [2] and the exporting of couple of
>>>>> functions in OF base code [3].
>>>>>
>>>>> With this in place, the remoteproc core need not be looking for the
>>>>> TI specific property anymore. I will submit the DTS changes once this
>>>>> makes it to mainline.
>>>>>
>>>>> regards
>>>>> Suman
>>>>>
>>>>> [1] https://patchwork.kernel.org/patch/9237767/
>>>>> [2] http://marc.info/?l=linux-kernel&m=146894341701010&w=2
>>>>> [3] https://patchwork.kernel.org/patch/9276181/
>>>>>
>>>>> .../devicetree/bindings/soc/ti/wkup_m3_ipc.txt | 9 ++++++--
>>>>> drivers/soc/ti/wkup_m3_ipc.c | 26 +++++++++++++++++++++-
>>>>> 2 files changed, 32 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/soc/ti/wkup_m3_ipc.txt b/Documentation/devicetree/bindings/soc/ti/wkup_m3_ipc.txt
>>>>> index 401550487ed6..2ea7dd91acff 100644
>>>>> --- a/Documentation/devicetree/bindings/soc/ti/wkup_m3_ipc.txt
>>>>> +++ b/Documentation/devicetree/bindings/soc/ti/wkup_m3_ipc.txt
>>>>> @@ -23,12 +23,17 @@ Required properties:
>>>>> with the Wakeup M3 processor
>>>>> - interrupts: Contains the interrupt information for the wkup_m3
>>>>> interrupt that signals the MPU.
>>>>> -- ti,rproc: phandle to the wkup_m3 rproc node so the IPC driver
>>>>> +- rprocs: phandle to the wkup_m3 rproc node so the IPC driver
>>>>> can boot it.
>>>>> - mboxes: phandles used by IPC framework to get correct mbox
>>>>> channel for communication. Must point to appropriate
>>>>> mbox_wkupm3 child node.
>>>>>
>>>>> +Deprecated properties:
>>>>> +----------------------
>>>>> +- ti,rproc: This property has been replaced with the "rprocs"
>>>>> + property.
>>>>> +
>>>>> Example:
>>>>> --------
>>>>> /* AM33xx */
>>>>> @@ -48,7 +53,7 @@ Example:
>>>>> compatible = "ti,am3352-wkup-m3-ipc";
>>>>> reg = <0x1324 0x24>;
>>>>> interrupts = <78>;
>>>>> - ti,rproc = <&wkup_m3>;
>>>>> + rprocs = <&wkup_m3>;
>>>>> mboxes = <&mailbox &mbox_wkupm3>;
>>>>> };
>>>>>
>>>>> diff --git a/drivers/soc/ti/wkup_m3_ipc.c b/drivers/soc/ti/wkup_m3_ipc.c
>>>>> index 8823cc81ae45..86085f9bf6a8 100644
>>>>> --- a/drivers/soc/ti/wkup_m3_ipc.c
>>>>> +++ b/drivers/soc/ti/wkup_m3_ipc.c
>>>>> @@ -1,7 +1,7 @@
>>>>> /*
>>>>> * AMx3 Wkup M3 IPC driver
>>>>> *
>>>>> - * Copyright (C) 2015 Texas Instruments, Inc.
>>>>> + * Copyright (C) 2015-2016 Texas Instruments, Inc.
>>>>> *
>>>>> * Dave Gerlach <d-gerlach at ti.com>
>>>>> *
>>>>> @@ -390,6 +390,8 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev)
>>>>> struct resource *res;
>>>>> struct task_struct *task;
>>>>> struct wkup_m3_ipc *m3_ipc;
>>>>> + struct property *nprop, *oprop;
>>>>> + const char nprop_name[] = "rprocs";
>>>>>
>>>>> m3_ipc = devm_kzalloc(dev, sizeof(*m3_ipc), GFP_KERNEL);
>>>>> if (!m3_ipc)
>>>>> @@ -415,6 +417,28 @@ static int wkup_m3_ipc_probe(struct platform_device *pdev)
>>>>> return ret;
>>>>> }
>>>>>
>>>>> + /* provide compatibility for older DTBs using ti,rproc property */
>>>>> + nprop = of_find_property(dev->of_node, "rprocs", NULL);
>>>>> + if (!nprop) {
>>>>> + oprop = of_find_property(dev->of_node, "ti,rproc", NULL);
>>>>> + if (!oprop) {
>>>>> + dev_err(&pdev->dev, "node is missing ti,rproc property\n");
>>>>> + return -ENODEV;
>>>>> + }
>>>>> +
>>>>> + nprop = kzalloc(sizeof(*nprop) + sizeof(nprop_name),
>>>>> + GFP_KERNEL);
>>>>> + if (!nprop)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + nprop->name = (char *)nprop + sizeof(*nprop);
>>>>> + snprintf(nprop->name, sizeof(nprop_name), nprop_name);
>>>>> + nprop->value = oprop->value;
>>>>> + nprop->length = oprop->length;
>>>>> + of_update_property(dev->of_node, nprop);
>>>>> + of_remove_property(dev->of_node, oprop);
>>>>> + }
>>>>> +
>>>>
>>>> +1 for getting the functionality out of core code.
>>>>
>>>> -100 for having to jump though all these hoops.
>>>>
>>>> If you are going to keep all of this, I would at least tuck it away in
>>>> a header file or something.
>>>
>>> Hiding it somewhere else won't make much of a difference though. If this
>>> is such an eyesore, we still can do this cleanly without jumping through
>>> these hoops. I would say this is a direct result of the removal of the
>>> rproc_get_by_phandle API in Patch 2 of your OF remoteproc series [2].
>>> That API is still an agnostic API. We can just add the of_rproc_get()
>>> convenience helper in your Patch 1 and drop patch 2 completely with no
>>> references to ti,rproc in the core code in the first place. The other
>>> option, if we do want to remove that API is to add an additional
>>> property name argument (NULL to mean "rprocs") to
>>> of_rproc_get_by_index(), and it can be hidden away under of_rproc_get()
>>> API - which is what I am expecting most of the new client users would
>>> use anyway.
>>>
>>> Once "rprocs" hits mainline, I will definitely switch over the
>>> wkup_m3_ipc nodes to use that standard property and can fix this driver
>>> independently.
>>>
>>
>> We're stuck with this problem all over the place, as the world continues
>> to evolve we will have issues with DT being static. This has been
>> discussed many times before and the suggested solution is always what
>> you implemented here - make the code deal with both versions, preferably
>> by patching.
>>
>> The fact that you had to export the of_ operations indicates that no-one
>> has tried this before and I'm happy you did. I'm however not happy about
>> the size of the chunk of code it takes to do this dance.
>>
>> I think for this to be practical we need to provide higher level
>> operations for DT modification - in this case a of_rename_property().
>>
>> @Rob, any comments on this?
>
> I agree. Pantelis submitted some helpers in this area a while back
> (for the changeset API IIRC). I believe they were mostly fine, but
> needed some users.
>
Is this the series you are talking about?
http://marc.info/?l=devicetree&m=146341689512653&w=2
It looks like that series is effective only when OF_DYNAMIC is enabled.
Probably a dumb question, but is this limited to DT Overlays? This
particular usage is a one-time change (first-time module is insmod'd)
mainly to provide compatibility for older DTBs, thereafter we wouldn't
be required to make any changes. It is definitely not a bulk update and
we don't want to unroll the changes even if we removed the module.
regards
Suman
More information about the linux-arm-kernel
mailing list