| Message ID | 20210402092049.479-4-nava.manne@xilinx.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable PM generic domain support | expand |
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? Thanks, Moritz
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. Regards, Navakishore.
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. > > Regards, > Navakishore.
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(-) > /** > @@ -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; Just a drive-by comment: you have PM usage counter imbalance here (double put). > 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; Johan
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.
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; }
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(-)