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

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

On Fri 09 Dec 03:42 PST 2016, Dwivedi, Avaneesh Kumar (avani) wrote:

> >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.
> ok. so i am going to add additional compatible string while also keeping
> existing compatible as below.

Yes, we need to keep the qcom,q6v5-pil and your change below looks to
get it right.

> Also compatible string i have changed but is it OK to keep
> resource instance named as "qdsp6v5_5_0_0_res" as below?
> 

Please update their naming based on the platform name as well, it will
cause less confusion in the future.

> +static const struct rproc_hexagon_res qdsp6v5_5_0_0_res = {
> +       .hexagon_mba_image = "mba.mbn",
> +};

A single empty line between each struct makes things easier to read.

> +static const struct rproc_hexagon_res qdsp6v5_5_1_1_res = {
> +       .hexagon_mba_image = "mba.b00",
> +};
>  static const struct of_device_id q6v5_of_match[] = {
> -       { .compatible = "qcom,q6v5-pil", },
> +       { .compatible = "qcom,q6v5-pil", .data = &qdsp6v5_5_0_0_res},
> +       { .compatible = "qcom,msm8916-mss-pil", .data = &qdsp6v5_5_0_0_res},
> +       { .compatible = "qcom,msm8974-mss-pil", .data = &qdsp6v5_5_1_1_res},
>         { },
>  };
[..]
> >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.
> In very few cases same hexagon chip is used in more than one msm platform
> i.e. one-to-many
> example  q6v56 1.8 is used on msm8952 as well as on msm8940, but generally
> it is one to one mapping with msm platform.

Okay, thanks for the summary.

In these rare cases we can have two compatibles referencing the same
struct.

> >
> >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"
> OK.
> >
> >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).
> so i will keep "qcom,msm8916-mss-pil" as well as "qcom,q6v5-pil" both in
> code.

Sound good.

Regards,
Bjorn