| Message ID | 20190813082442.25796-5-mkshah@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Add RSC power domain support | expand |
Quoting Maulik Shah (2019-08-13 01:24:42) > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > index e278fc11fe5c..bd8e9f1a43b4 100644 > --- a/drivers/soc/qcom/rpmh-rsc.c > +++ b/drivers/soc/qcom/rpmh-rsc.c > @@ -498,6 +498,32 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg) > return ret; > } > > +/** > + * rpmh_rsc_ctrlr_is_idle: Check if any of the AMCs are busy. > + * > + * @drv: The controller > + * > + * Returns false if the TCSes are engaged in handling requests, > + * True if controller is idle. > + */ > +static bool rpmh_rsc_ctrlr_is_idle(struct rsc_drv *drv) > +{ > + int m; > + struct tcs_group *tcs = get_tcs_of_type(drv, ACTIVE_TCS); > + bool ret = true; > + > + spin_lock(&drv->lock); > + for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) { > + if (!tcs_is_free(drv, m)) { Isn't this a copy of an existing function in the rpmh driver? > + ret = false; > + break; > + } > + } > + spin_unlock(&drv->lock); > + > + return ret; > +} > + > /** > * rpmh_rsc_write_ctrl_data: Write request to the controller > * > @@ -521,6 +547,65 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) > return tcs_ctrl_write(drv, msg); > } > > +int rpmh_domain_power_off(struct generic_pm_domain *rsc_pd) > +{ > + struct rsc_drv *drv = container_of(rsc_pd, struct rsc_drv, rsc_pd); > + int ret = 0; > + > + /* > + * RPMh domain can not be powered off when there is pending ACK for > + * ACTIVE_TCS request. Exit when controller is busy. > + */ > + > + ret = rpmh_rsc_ctrlr_is_idle(drv); > + if (!ret) > + goto exit; return 0? Shouldn't it return some negative value? > + > + ret = rpmh_flush(&drv->client); > + if (ret) > + goto exit; Why not just return rpmh_flush(...)? The usage of goto in this function is entirely unnecessary. > + > +exit: > + return ret; > +} > + > +static int rpmh_probe_power_domain(struct platform_device *pdev, > + struct rsc_drv *drv) > +{ > + int ret = -ENOMEM; > + struct generic_pm_domain *rsc_pd = &drv->rsc_pd; > + struct device_node *dn = pdev->dev.of_node; > + > + rsc_pd->name = kasprintf(GFP_KERNEL, "%s", dn->name); > + if (!rsc_pd->name) > + goto exit; return -ENOMEM; > + > + rsc_pd->name = kbasename(rsc_pd->name); > + rsc_pd->power_off = rpmh_domain_power_off; > + rsc_pd->flags |= GENPD_FLAG_IRQ_SAFE; > + > + ret = pm_genpd_init(rsc_pd, NULL, false); > + if (ret) > + goto free_name; > + > + ret = of_genpd_add_provider_simple(dn, rsc_pd); > + if (ret) > + goto remove_pd; > + > + pr_debug("init PM domain %s\n", rsc_pd->name); > + > + return ret; ret = of_genpd_add_provider_simple(...) if (!ret) return 0; Drop the pr_debug(), it's not useful. > + > +remove_pd: > + pm_genpd_remove(rsc_pd); > + > +free_name: > + kfree(rsc_pd->name); > + > +exit: > + return ret; Please remove newlines between labels above.
On 8/14/2019 11:55 PM, Stephen Boyd wrote: > Quoting Maulik Shah (2019-08-13 01:24:42) >> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c >> index e278fc11fe5c..bd8e9f1a43b4 100644 >> --- a/drivers/soc/qcom/rpmh-rsc.c >> +++ b/drivers/soc/qcom/rpmh-rsc.c >> @@ -498,6 +498,32 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg) >> return ret; >> } >> >> +/** >> + * rpmh_rsc_ctrlr_is_idle: Check if any of the AMCs are busy. >> + * >> + * @drv: The controller >> + * >> + * Returns false if the TCSes are engaged in handling requests, >> + * True if controller is idle. >> + */ >> +static bool rpmh_rsc_ctrlr_is_idle(struct rsc_drv *drv) >> +{ >> + int m; >> + struct tcs_group *tcs = get_tcs_of_type(drv, ACTIVE_TCS); >> + bool ret = true; >> + >> + spin_lock(&drv->lock); >> + for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) { >> + if (!tcs_is_free(drv, m)) { > Isn't this a copy of an existing function in the rpmh driver? No. The changes to add this were previously posted but did not went it. >> + ret = false; >> + break; >> + } >> + } >> + spin_unlock(&drv->lock); >> + >> + return ret; >> +} >> + >> /** >> * rpmh_rsc_write_ctrl_data: Write request to the controller >> * >> @@ -521,6 +547,65 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) >> return tcs_ctrl_write(drv, msg); >> } >> >> +int rpmh_domain_power_off(struct generic_pm_domain *rsc_pd) >> +{ >> + struct rsc_drv *drv = container_of(rsc_pd, struct rsc_drv, rsc_pd); >> + int ret = 0; >> + >> + /* >> + * RPMh domain can not be powered off when there is pending ACK for >> + * ACTIVE_TCS request. Exit when controller is busy. >> + */ >> + >> + ret = rpmh_rsc_ctrlr_is_idle(drv); >> + if (!ret) >> + goto exit; > return 0? Shouldn't it return some negative value? Done. >> + >> + ret = rpmh_flush(&drv->client); >> + if (ret) >> + goto exit; > Why not just return rpmh_flush(...)? > > The usage of goto in this function is entirely unnecessary. Done. >> + >> +exit: >> + return ret; >> +} >> + >> +static int rpmh_probe_power_domain(struct platform_device *pdev, >> + struct rsc_drv *drv) >> +{ >> + int ret = -ENOMEM; >> + struct generic_pm_domain *rsc_pd = &drv->rsc_pd; >> + struct device_node *dn = pdev->dev.of_node; >> + >> + rsc_pd->name = kasprintf(GFP_KERNEL, "%s", dn->name); >> + if (!rsc_pd->name) >> + goto exit; > return -ENOMEM; Done. >> + >> + rsc_pd->name = kbasename(rsc_pd->name); >> + rsc_pd->power_off = rpmh_domain_power_off; >> + rsc_pd->flags |= GENPD_FLAG_IRQ_SAFE; >> + >> + ret = pm_genpd_init(rsc_pd, NULL, false); >> + if (ret) >> + goto free_name; >> + >> + ret = of_genpd_add_provider_simple(dn, rsc_pd); >> + if (ret) >> + goto remove_pd; >> + >> + pr_debug("init PM domain %s\n", rsc_pd->name); >> + >> + return ret; > ret = of_genpd_add_provider_simple(...) > if (!ret) > return 0; > > Drop the pr_debug(), it's not useful. Done. >> + >> +remove_pd: >> + pm_genpd_remove(rsc_pd); >> + >> +free_name: >> + kfree(rsc_pd->name); >> + >> +exit: >> + return ret; > Please remove newlines between labels above. Done.
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h index 6eec32b97f83..3736c148effc 100644 --- a/drivers/soc/qcom/rpmh-internal.h +++ b/drivers/soc/qcom/rpmh-internal.h @@ -8,6 +8,7 @@ #define __RPM_INTERNAL_H__ #include <linux/bitmap.h> +#include <linux/pm_domain.h> #include <soc/qcom/tcs.h> #define TCS_TYPE_NR 4 @@ -102,6 +103,7 @@ struct rsc_drv { DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR); spinlock_t lock; struct rpmh_ctrlr client; + struct generic_pm_domain rsc_pd; }; int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg); diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index e278fc11fe5c..bd8e9f1a43b4 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -498,6 +498,32 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const struct tcs_request *msg) return ret; } +/** + * rpmh_rsc_ctrlr_is_idle: Check if any of the AMCs are busy. + * + * @drv: The controller + * + * Returns false if the TCSes are engaged in handling requests, + * True if controller is idle. + */ +static bool rpmh_rsc_ctrlr_is_idle(struct rsc_drv *drv) +{ + int m; + struct tcs_group *tcs = get_tcs_of_type(drv, ACTIVE_TCS); + bool ret = true; + + spin_lock(&drv->lock); + for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) { + if (!tcs_is_free(drv, m)) { + ret = false; + break; + } + } + spin_unlock(&drv->lock); + + return ret; +} + /** * rpmh_rsc_write_ctrl_data: Write request to the controller * @@ -521,6 +547,65 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg) return tcs_ctrl_write(drv, msg); } +int rpmh_domain_power_off(struct generic_pm_domain *rsc_pd) +{ + struct rsc_drv *drv = container_of(rsc_pd, struct rsc_drv, rsc_pd); + int ret = 0; + + /* + * RPMh domain can not be powered off when there is pending ACK for + * ACTIVE_TCS request. Exit when controller is busy. + */ + + ret = rpmh_rsc_ctrlr_is_idle(drv); + if (!ret) + goto exit; + + ret = rpmh_flush(&drv->client); + if (ret) + goto exit; + +exit: + return ret; +} + +static int rpmh_probe_power_domain(struct platform_device *pdev, + struct rsc_drv *drv) +{ + int ret = -ENOMEM; + struct generic_pm_domain *rsc_pd = &drv->rsc_pd; + struct device_node *dn = pdev->dev.of_node; + + rsc_pd->name = kasprintf(GFP_KERNEL, "%s", dn->name); + if (!rsc_pd->name) + goto exit; + + rsc_pd->name = kbasename(rsc_pd->name); + rsc_pd->power_off = rpmh_domain_power_off; + rsc_pd->flags |= GENPD_FLAG_IRQ_SAFE; + + ret = pm_genpd_init(rsc_pd, NULL, false); + if (ret) + goto free_name; + + ret = of_genpd_add_provider_simple(dn, rsc_pd); + if (ret) + goto remove_pd; + + pr_debug("init PM domain %s\n", rsc_pd->name); + + return ret; + +remove_pd: + pm_genpd_remove(rsc_pd); + +free_name: + kfree(rsc_pd->name); + +exit: + return ret; +} + static int rpmh_probe_tcs_config(struct platform_device *pdev, struct rsc_drv *drv) { @@ -650,6 +735,17 @@ static int rpmh_rsc_probe(struct platform_device *pdev) if (ret) return ret; + /* + * Power domain is not required for controllers that support 'solver' + * mode where they can be in autonomous mode executing low power mode + * to power down. + */ + if (of_property_read_bool(dn, "#power-domain-cells")) { + ret = rpmh_probe_power_domain(pdev, drv); + if (ret) + return ret; + } + spin_lock_init(&drv->lock); bitmap_zero(drv->tcs_in_use, MAX_TCS_NR);
Add RSC power domain support. RSC is top level power domain in hireachical CPU LPM modes. Once the rsc domain is down flush all cached sleep and wake votes from controller. Signed-off-by: Maulik Shah <mkshah@codeaurora.org> --- drivers/soc/qcom/rpmh-internal.h | 2 + drivers/soc/qcom/rpmh-rsc.c | 96 ++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+)