Re: [PATCH v2 4/9] remoteproc: qcom_q6v5_mss: Add modem support on MSM8226

[Stephan Gerhold <stephan.gerhold@xxxxxxxxxx>]

On Sun, Jan 26, 2025 at 09:57:23PM +0100, Luca Weiss wrote:
> Add support for the external power block headswitch register needed by
> MSM8226 and some other qcom platforms.
> 
> Co-developed-by: Matti Lehtimäki <matti.lehtimaki@xxxxxxxxx>
> Signed-off-by: Matti Lehtimäki <matti.lehtimaki@xxxxxxxxx>
> Signed-off-by: Luca Weiss <luca@xxxxxxxxxxxx>
> ---
> Changes in v2:
>   - Remove cx-supply from MSM8226 "fallback_proxy_supply" because it's
>     always used as power domain
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 117 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 117 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index e2523b01febf393abfe50740a68b85a04011293c..ef85c9843a12acb0e4e9ee6167c34d4981d4dc8a 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -134,6 +134,11 @@
>  #define BOOT_FSM_TIMEOUT                10000
>  #define BHS_CHECK_MAX_LOOPS             200
>  
> +/* External power block headswitch */
> +#define EXTERNAL_BHS_ON			BIT(0)
> +#define EXTERNAL_BHS_STATUS		BIT(4)
> +#define EXTERNAL_BHS_TIMEOUT_US		50
> +
>  struct reg_info {
>  	struct regulator *reg;
>  	int uV;
> @@ -161,6 +166,7 @@ struct rproc_hexagon_res {
> [...]
> @@ -1871,6 +1897,36 @@ static void q6v5_pds_detach(struct q6v5 *qproc, struct device **pds,
>  		dev_pm_domain_detach(pds[i], false);
>  }
>  
> +static int q6v5_external_bhs_enable(struct q6v5 *qproc)
> +{
> +	u32 val;
> +	int ret = 0;
> +
> +	/*
> +	 * Enable external power block headswitch and wait for it to
> +	 * stabilize
> +	 */
> +	regmap_update_bits(qproc->conn_map, qproc->ext_bhs,
> +			   EXTERNAL_BHS_ON, 1);

Nitpick: regmap_update_bits() doesn't take a bool (0/1) as last value,
it's the actual value. The 1 as last argument is essentially a magic
number here that happens to be equal to EXTERNAL_BHS_ON = BIT(0).

This should be

	regmap_update_bits(..., EXTERNAL_BHS_ON, EXTERNAL_BHS_ON);

or even better just

	regmap_set_bits(..., EXTERNAL_BHS_ON);

> +
> +	ret = regmap_read_poll_timeout(qproc->conn_map, qproc->ext_bhs,
> +				       val, val & EXTERNAL_BHS_STATUS,
> +				       1, EXTERNAL_BHS_TIMEOUT_US);
> +
> +	if (ret) {
> +		dev_err(qproc->dev, "External BHS timed out\n");
> +		ret = -ETIMEDOUT;
> +	}
> +
> +	return ret;
> +}
> +
> +static void q6v5_external_bhs_disable(struct q6v5 *qproc)
> +{
> +	regmap_update_bits(qproc->conn_map, qproc->ext_bhs,
> +			   EXTERNAL_BHS_ON, 0);

	regmap_clear_bits(..., EXTERNAL_BHS_ON);

> +}
> +
>  static int q6v5_init_reset(struct q6v5 *qproc)
>  {
>  	qproc->mss_restart = devm_reset_control_get_exclusive(qproc->dev,
> @@ -2021,6 +2077,7 @@ static int q6v5_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, qproc);
>  
>  	qproc->has_qaccept_regs = desc->has_qaccept_regs;
> +	qproc->has_ext_bhs_reg = desc->has_ext_bhs_reg;
>  	qproc->has_ext_cntl_regs = desc->has_ext_cntl_regs;
>  	qproc->has_vq6 = desc->has_vq6;
>  	qproc->has_spare_reg = desc->has_spare_reg;
> @@ -2079,6 +2136,14 @@ static int q6v5_probe(struct platform_device *pdev)
>  		qproc->proxy_pd_count = ret;
>  	}
>  
> +	if (qproc->has_ext_bhs_reg) {
> +		ret = q6v5_external_bhs_enable(qproc);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "Failed to enable external BHS.\n");

Since the only possible error condition inside
q6v5_external_bhs_enable() is the timeout condition that is already
logged, you might as well drop this error message.

Thanks,
Stephan