On Fri 20 Jan 08:10 PST 2017, Avaneesh Kumar Dwivedi wrote:
> This patch make resource handling generic in adsp remoteproc driver.
> Resources which were initialized with hard definition are being
> initialized now based on compatible string. Generic way of resource
> initialization and handling is required so that slpi processor boot
> support can also be enabled with same driver.
>
I think adding a "generic way of resource initialization and handling"
is overkill for this driver, at this point in time at least.
> Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@xxxxxxxxxxxxxx>
> ---
> drivers/remoteproc/qcom_adsp_pil.c | 259 +++++++++++++++++++++++++++++--------
> 1 file changed, 208 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_adsp_pil.c b/drivers/remoteproc/qcom_adsp_pil.c
> index 43a4ed2..ab2632b 100644
> --- a/drivers/remoteproc/qcom_adsp_pil.c
> +++ b/drivers/remoteproc/qcom_adsp_pil.c
> @@ -32,9 +32,26 @@
> #include "qcom_mdt_loader.h"
> #include "remoteproc_internal.h"
>
> -#define ADSP_CRASH_REASON_SMEM 423
> -#define ADSP_FIRMWARE_NAME "adsp.mdt"
> -#define ADSP_PAS_ID 1
> +struct reg_info {
> + struct regulator *reg;
> + int uV;
> + int uA;
> +};
> +
> +struct regulator_res {
> + const char *supply;
> + int uV;
> + int uA;
> +};
As far as I can see we have 2 variables:
1) Do we have px-supply?
We could just always devm_regualator_get("px"), in the adsp case this
would give us a dummy regulator that we can still
regulator_enable/disable. So we can keep the code unconditional.
If we want to save the reader of the kernel log a printout about a dummy
voltage we can carry a "bool has_px" in the adsp_data.
The mechanism for controlling corner voltages will be something other
than the regulator api, so let's not design the driver for
voltage/current selection yet.
2) Do we have aggre2_noc clock?
This info we can carry with a bool in the data struct, making the
devm_clk_get("aggre2") depend on this or leave the clk NULL - also
calling this unconditionally.
> +
> +struct generic_rproc_res {
Please name this "adsp_data".
> + int crash_reason_smem;
> + const char *firmware_name;
> + int fw_pas_id;
"pas_id" is enough.
> + struct regulator_res reg_res[3];
> + char **clocks_name;
> +};
> +
>
> struct qcom_adsp {
> struct device *dev;
> @@ -49,9 +66,13 @@ struct qcom_adsp {
> struct qcom_smem_state *state;
> unsigned stop_bit;
>
> - struct clk *xo;
> + struct clk *clocks[3];
> + int clk_count;
> + struct reg_info regulators[3];
> + int reg_count;
>
> - struct regulator *cx_supply;
> + int fw_pas_id;
"pas_id"
> + int crash_reason_smem;
>
> struct completion start_done;
> struct completion stop_done;
> @@ -62,6 +83,136 @@ struct qcom_adsp {
> size_t mem_size;
> };
>
[..]
>
> -static int adsp_init_clock(struct qcom_adsp *adsp)
> -{
> - int ret;
> -
> - adsp->xo = devm_clk_get(adsp->dev, "xo");
> - if (IS_ERR(adsp->xo)) {
> - ret = PTR_ERR(adsp->xo);
> - if (ret != -EPROBE_DEFER)
> - dev_err(adsp->dev, "failed to get xo clock");
> - return ret;
> - }
> -
Just do:
if (adsp->has_aggre2_clk) {
adsp->aggre2_clk = devm_clk_get(..);
...
}
> - return 0;
> -}
> -
> -static int adsp_init_regulator(struct qcom_adsp *adsp)
> -{
> - adsp->cx_supply = devm_regulator_get(adsp->dev, "cx");
> - if (IS_ERR(adsp->cx_supply))
> - return PTR_ERR(adsp->cx_supply);
> -
> - regulator_set_load(adsp->cx_supply, 100000);
If you just request "px" unconditionally here we will get a print in the
log informing us that we got a dummy regulator. If we want to be fancy
and not have that you can do a boolean.
> -
> - return 0;
> -}
> -
[..]
>
> +static const struct generic_rproc_res adsp_resource_init = {
> + .crash_reason_smem = 423,
> + .firmware_name = "adsp.mdt",
> + .fw_pas_id = 1,
> + .reg_res = (struct regulator_res[]) {
> + {
> + .supply = "vdd_cx",
> + },
> + {},
> + {}
> + },
> + .clocks_name = (char*[]){
> + "xo",
> + NULL
> + },
> +};
Please leave this a empty line between these.
> static const struct of_device_id adsp_of_match[] = {
> - { .compatible = "qcom,msm8974-adsp-pil" },
> - { .compatible = "qcom,msm8996-adsp-pil" },
> + { .compatible = "qcom,msm8974-adsp-pil", .data = &adsp_resource_init},
> + { .compatible = "qcom,msm8996-adsp-pil", .data = &adsp_resource_init},
> { },
> };
> MODULE_DEVICE_TABLE(of, adsp_of_match);
Regards,
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
