Re: [RESEND PATCH v4 1/7] remoteproc: qcom: Add and initialize private data for hexagon dsp.

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote:

> Resource string's specific to version of hexagon chip are initialized
> to be passed to probe for various resource init purpose.
> Also compatible string introduced are added to documentation.
> 

Overall I think the series looks good now, will comment on individual
things of each patch, but I'm generally happy about how things look.


I would however like to see a slight restructuring between the patches.

Rather than adding regulators, clocks and version to the res-struct in
patch 1 and then add the code using these later I would like you to
introduce the smallest possible struct here and then add each part in
the relevant patch. And finish off with adding the msm8996 compatible,
once all the pieces are in place.

So in this first patch I would suggest that you add the msm8974 and
msm8916 compatibles, a res-struct containing only hexagon_mba_image and
the change from patch 2 where you change rproc_alloc() to use the
hexagon_mba_image.

Then in the regulator patch add the regulators for msm8974 and msm8916,
same with clocks in the clock patch and then add the hexagon_ver, the
associated changes and the msm8996 compatible in one patch.

That way each patch add a single consistent chunk of the changes.

> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org>
> ---
>  .../devicetree/bindings/remoteproc/qcom,q6v5.txt   |  2 +
>  drivers/remoteproc/qcom_q6v5_pil.c                 | 61 +++++++++++++++++++++-
>  2 files changed, 62 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> index 57cb49e..d4c14f0 100644
> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
> @@ -8,6 +8,8 @@ on the Qualcomm Hexagon core.
>  	Value type: <string>
>  	Definition: must be one of:
>  		    "qcom,q6v5-pil"
> +		"qcom,q6v5-5-1-1-pil"
> +		"qcom,q6v56-1-5-0-pil"

The more I look at these numbers and what's used downstream the more
confused I get and perhaps more important, I can't find any
documentation mentioning these numbers.

As far as I can see these numbers are 1:1 with specific platforms, which
we use as part of other bindings. So I think we should follow the naming
scheme we use for e.g. the ADSP PIL.

And let's replace the q6v5 part with "mss", as e.g. msm8974 adsp also is
a "q6v5".

So please add:
"qcom,msm8916-mss-pil",
"qcom,msm8974-mss-pil",
"qcom,msm8996-mss-pil"

The compatible "qcom,q6v5-pil" is already introduced in the
msm8916.dtsi, so make that compatible be equivalent to
"qcom,msm8916-mss-pil" (but let's have both for clarity).

>  
>  - reg:
>  	Usage: required
> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
> index 2e0caaa..3360312 100644
> --- a/drivers/remoteproc/qcom_q6v5_pil.c
> +++ b/drivers/remoteproc/qcom_q6v5_pil.c
> @@ -30,6 +30,7 @@
>  #include <linux/reset.h>
>  #include <linux/soc/qcom/smem.h>
>  #include <linux/soc/qcom/smem_state.h>
> +#include <linux/of_device.h>
>  
>  #include "remoteproc_internal.h"
>  #include "qcom_mdt_loader.h"
> @@ -93,6 +94,22 @@
>  #define QDSS_BHS_ON			BIT(21)
>  #define QDSS_LDO_BYP			BIT(22)
>  
> +struct rproc_hexagon_res {
> +	char *hexagon_mba_image;

const

> +	char **proxy_reg_string;
> +	char **active_reg_string;
> +	int proxy_uV_uA[3][2];
> +	int active_uV_uA[1][2];

Let's group these into a:

struct qcom_mss_reg_res {
	const char *supply;
	int uA;
	int uV;
};

Then the res definitions below becomes:

satic const struct rproc_hexagon_res msm8916_res = {
	.proxy_regs = (struct qcom_mss_reg_res[]) {
		{
			.supply = "mx",
		},
		{
			.supply = "cx",
			.uA = 100000,
		},
		{
			.supply = "pll",
			.uA = 100000,
		},
		{ NULL }
	},
	...
};

> +	char **proxy_clk_string;
> +	char **active_clk_string;
> +	int hexagon_ver;
> +};
> +
> +struct reg_info {
> +	struct regulator *reg;
> +	int uV;
> +	int uA;
> +};
>  struct q6v5 {
>  	struct device *dev;
>  	struct rproc *rproc;
> @@ -131,6 +148,12 @@ struct q6v5 {
>  };
>  
>  enum {
> +	Q6V5_5_0_0, /*hexagon on msm8916*/
> +	Q6V5_5_1_1, /*hexagon on msm8974*/
> +	Q5V56_1_5_0, /*hexagon on msm8996*/

As I said above, this turns out to be confusing. Name them based on
platform instead. Something like Q6V5_MSM8916

> +};
> +

Regards,
Bjorn