Re: [PATCH] soc: ti: wkup_m3_ipc: switch to using remoteproc OF infrastructure

[Suman Anna <s-anna@ti.com>]

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@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@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@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