Messages in this thread | | | Subject | Re: [PATCH 3/3] fpga: region: Adds runtime PM support | From | Tom Rix <> | Date | Wed, 2 Jun 2021 13:39:10 -0700 |
| |
On 6/2/21 3:48 AM, Nava kishore Manne wrote: > Ping! > >> -----Original Message----- >> From: Nava kishore Manne >> Sent: Monday, April 19, 2021 10:49 AM >> To: Moritz Fischer <mdf@kernel.org> >> Cc: trix@redhat.com; robh+dt@kernel.org; Michal Simek >> <michals@xilinx.com>; linux-fpga@vger.kernel.org; >> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- >> kernel@lists.infradead.org; git <git@xilinx.com> >> Subject: RE: [PATCH 3/3] fpga: region: Adds runtime PM support >> >> Hi Moritz, >> >> Please find my response inline. >> >>> -----Original Message----- >>> From: Moritz Fischer <mdf@kernel.org> >>> Sent: Saturday, April 10, 2021 3:01 AM >>> To: Nava kishore Manne <navam@xilinx.com> >>> Cc: mdf@kernel.org; trix@redhat.com; robh+dt@kernel.org; Michal Simek >>> <michals@xilinx.com>; linux-fpga@vger.kernel.org; >>> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- >>> kernel@lists.infradead.org; git <git@xilinx.com> >>> Subject: Re: [PATCH 3/3] fpga: region: Adds runtime PM support >>> >>> On Fri, Apr 02, 2021 at 02:50:49PM +0530, Nava kishore Manne wrote: >>>> Adds support to handle FPGA/PL power domain. With this patch, the PL >>>> power domain will be turned on before loading the bitstream and >>>> turned off while removing/unloading the bitstream using overlays. >>>> This can be achieved by adding the runtime PM support. >>>> >>>> Signed-off-by: Nava kishore Manne <nava.manne@xilinx.com> >>>> --- >>>> drivers/fpga/of-fpga-region.c | 18 +++++++++++++++++- >>>> 1 file changed, 17 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/fpga/of-fpga-region.c >>>> b/drivers/fpga/of-fpga-region.c index e405309baadc..35fc2f3d4bd8 >>>> 100644 >>>> --- a/drivers/fpga/of-fpga-region.c >>>> +++ b/drivers/fpga/of-fpga-region.c >>>> @@ -15,6 +15,7 @@ >>>> #include <linux/of_platform.h> >>>> #include <linux/slab.h> >>>> #include <linux/spinlock.h> >>>> +#include <linux/pm_runtime.h> >>>> >>>> static const struct of_device_id fpga_region_of_match[] = { >>>> { .compatible = "fpga-region", }, >>>> @@ -302,6 +303,7 @@ static int >>>> of_fpga_region_notify_pre_apply(struct >>> fpga_region *region, >>>> } >>>> >>>> region->info = info; >>>> + pm_runtime_get_sync(dev->parent); >>>> ret = fpga_region_program_fpga(region); >>>> if (ret) { >>>> /* error; reject overlay */ >>>> @@ -324,10 +326,13 @@ static int >>>> of_fpga_region_notify_pre_apply(struct fpga_region *region, static >>>> void >>> of_fpga_region_notify_post_remove(struct fpga_region *region, >>>> struct of_overlay_notify_data >>> *nd) { >>>> + struct device *dev = ®ion->dev; >>>> + >>>> fpga_bridges_disable(®ion->bridge_list); >>>> fpga_bridges_put(®ion->bridge_list); >>>> fpga_image_info_free(region->info); >>>> region->info = NULL; >>>> + pm_runtime_put(dev->parent); >>>> } >>>> >>>> /** >>>> @@ -411,9 +416,16 @@ static int of_fpga_region_probe(struct >>> platform_device *pdev) >>>> goto eprobe_mgr_put; >>>> } >>>> >>>> + pm_runtime_enable(&pdev->dev); >>>> + ret = pm_runtime_get_sync(&pdev->dev); >>>> + if (ret < 0) >>>> + goto err_pm; >>>> + >>>> + pm_runtime_put(&pdev->dev); >>>> + >>>> ret = fpga_region_register(region); >>>> if (ret) >>>> - goto eprobe_mgr_put; >>>> + goto err_pm; >>>> >>>> of_platform_populate(np, fpga_region_of_match, NULL, ®ion- >>>> dev); >>>> platform_set_drvdata(pdev, region); @@ -422,6 +434,9 @@ static >>>> int of_fpga_region_probe(struct platform_device *pdev) >>>> >>>> return 0; >>>> >>>> +err_pm: >>>> + pm_runtime_put(&pdev->dev); >>>> + pm_runtime_disable(&pdev->dev); >>>> eprobe_mgr_put: >>>> fpga_mgr_put(mgr); >>>> return ret; >>>> @@ -434,6 +449,7 @@ static int of_fpga_region_remove(struct >>>> platform_device *pdev) >>>> >>>> fpga_region_unregister(region); >>>> fpga_mgr_put(mgr); >>>> + pm_runtime_disable(region->dev.parent); >>>> >>>> return 0; >>>> } >>>> -- >>>> 2.18.0 >>>> >>> Does this add a dependency on CONFIG_PM? >>> >> Yes, the newly added functionality has a dependency on CONFIG_PM if it's >> disabled the expected power domain (in our cases it's PL) will not powered >> ON/OFF dynamically at runtime and this is as an expected behavior. Other >> than this it will not impact the any of the driver existing functionality.
This looks like a general change for a specific board.
Why can't this be done at the board level ?
Tom
>> >> Regards, >> Navakishore.
| |