Linux-PM Archive on lore.kernel.org
 help / color / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Maulik Shah <mkshah@codeaurora.org>,
	agross@kernel.org, david.brown@linaro.org,
	linux-arm-msm@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
	bjorn.andersson@linaro.org, evgreen@chromium.org,
	dianders@chromium.org, rnayak@codeaurora.org,
	ilina@codeaurora.org, lsrao@codeaurora.org,
	ulf.hansson@linaro.org, Maulik Shah <mkshah@codeaurora.org>
Subject: Re: [PATCH v2 4/6] drivers: qcom: rpmh-rsc: Add RSC power domain support
Date: Thu, 05 Sep 2019 10:32:45 -0700
Message-ID: <5d7146be.1c69fb81.38760.7fb8@mx.google.com> (raw)
In-Reply-To: <20190823081703.17325-5-mkshah@codeaurora.org>

Quoting Maulik Shah (2019-08-23 01:17:01)
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index e278fc11fe5c..884b39599e8f 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,

Please use kernel-doc style for returns here.

> + *  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);

I think these need to be irqsave/restore still.

> +       for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
> +               if (!tcs_is_free(drv, m)) {

This snippet is from tcs_invalidate(). Please collapse it into some sort
of function or macro like for_each_tcs().

> +                       ret = false;
> +                       break;
> +               }
> +       }
> +       spin_unlock(&drv->lock);
> +
> +       return ret;
> +}
> +
>  /**
>   * rpmh_rsc_write_ctrl_data: Write request to the controller
>   *
> @@ -521,6 +547,53 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
>         return tcs_ctrl_write(drv, msg);
>  }
>  
> +static int rpmh_domain_power_off(struct generic_pm_domain *rsc_pd)
> +{
> +       struct rsc_drv *drv = container_of(rsc_pd, struct rsc_drv, rsc_pd);
> +
> +       /*
> +        * RPMh domain can not be powered off when there is pending ACK for
> +        * ACTIVE_TCS request. Exit when controller is busy.
> +        */
> +

Nitpick: Remove this extra newline.

> +       if (!rpmh_rsc_ctrlr_is_idle(drv))
> +               return -EBUSY;
> +
> +       return rpmh_flush(&drv->client);
> +}
> +
> +static int rpmh_probe_power_domain(struct platform_device *pdev,
> +                                  struct rsc_drv *drv)
> +{
> +       int ret;
> +       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);

Maybe use devm_kasprintf?

> +       if (!rsc_pd->name)
> +               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;
> +
> +       return ret;
> +
> +remove_pd:
> +       pm_genpd_remove(rsc_pd);
> +free_name:
> +       kfree(rsc_pd->name);

And then drop this one?

> +       return ret;
> +}
> +
>  static int rpmh_probe_tcs_config(struct platform_device *pdev,
>                                  struct rsc_drv *drv)
>  {
> @@ -650,6 +723,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);

What happens if it fails later on? The genpd provider is still sitting
around and needs to be removed on probe failure later on in this
function. It would be nicer if there wasn't another function to probe
the power domain and it was just inlined here actually. That way we
don't have to wonder about what's going on across two blocks of code.


  reply index

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-23  8:16 [PATCH v2 0/6] " Maulik Shah
2019-08-23  8:16 ` [PATCH v2 1/6] drivers: qcom: rpmh: fix macro to accept NULL argument Maulik Shah
2019-08-23  8:16 ` [PATCH v2 2/6] drivers: qcom: rpmh: remove rpmh_flush export Maulik Shah
2019-08-23  8:17 ` [PATCH v2 3/6] dt-bindings: soc: qcom: Add RSC power domain specifier Maulik Shah
2019-08-27 22:32   ` Rob Herring
2019-09-03  8:44     ` Maulik Shah
2019-08-23  8:17 ` [PATCH v2 4/6] drivers: qcom: rpmh-rsc: Add RSC power domain support Maulik Shah
2019-09-05 17:32   ` Stephen Boyd [this message]
2019-08-23  8:17 ` [PATCH v2 5/6] arm64: dts: Convert to the hierarchical CPU topology layout for sdm845 Maulik Shah
2019-08-23  8:17 ` [PATCH v2 6/6] arm64: dts: Add rsc power domain " Maulik Shah
2019-09-05 17:33   ` Stephen Boyd
2020-01-21 19:05 ` [PATCH v2 0/6] Add RSC power domain support Matthias Kaehlcke

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5d7146be.1c69fb81.38760.7fb8@mx.google.com \
    --to=swboyd@chromium.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=david.brown@linaro.org \
    --cc=dianders@chromium.org \
    --cc=evgreen@chromium.org \
    --cc=ilina@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=lsrao@codeaurora.org \
    --cc=mkshah@codeaurora.org \
    --cc=rnayak@codeaurora.org \
    --cc=ulf.hansson@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-PM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pm/0 linux-pm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pm linux-pm/ https://lore.kernel.org/linux-pm \
		linux-pm@vger.kernel.org
	public-inbox-index linux-pm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git