On Fri, Aug 12, 2016 at 11:43 AM, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx> 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@xxxxxx> >> >> --- >> >> 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@xxxxxx> >> >> * >> >> @@ -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. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
