| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 22 Dec 2016 17:04:28 -0800
From: Bjorn Andersson <bjorn.andersson@...aro.org>
To: Avaneesh Kumar Dwivedi <akdwived@...eaurora.org>
Cc: sboyd@...eaurora.org, agross@...eaurora.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v5 6/7] remoteproc: qcom: Implement Hexagon core specific
changes.
On Thu 15 Dec 04:21 PST 2016, Avaneesh Kumar Dwivedi wrote:
> +/* QDSP6v56 parameters */
> +#define QDSP6v56_LDO_BYP BIT(25)
> +#define QDSP6v56_BHS_ON BIT(24)
> +#define QDSP6v56_CLAMP_WL BIT(21)
> +#define QDSP6v56_CLAMP_QMC_MEM BIT(22)
> +#define HALT_CHECK_MAX_LOOPS 200
> +#define QDSP6SS_XO_CBCR 0x0038
Indent these with tabs, like the others.
> +#define QDSP6SS_ACC_OVERRIDE_VAL 0x20
> +
> struct reg_info {
> struct regulator *reg;
> int uV;
> @@ -111,6 +123,7 @@ struct rproc_hexagon_res {
> struct qcom_mss_reg_res active_supply[2];
> char **proxy_clk_string;
> char **active_clk_string;
> + int hexagon_ver;
> };
>
> struct q6v5 {
> @@ -152,8 +165,14 @@ struct q6v5 {
> phys_addr_t mpss_reloc;
> void *mpss_region;
> size_t mpss_size;
> + int hexagon_ver;
Please name this "version" instead, there's little confusion to what it
is.
> };
>
> +enum {
> + MSS_MSM8916, /*hexagon on msm8916*/
> + MSS_MSM8974, /*hexagon on msm8974*/
> + MSS_MSM8996, /*hexagon on msm8996*/
You can skip the comments now.
> +};
>
> static int q6v5_regulator_init(struct device *dev, struct reg_info *regs,
> const struct qcom_mss_reg_res *reg_res)
> @@ -341,35 +360,107 @@ static int q6v5_rmb_mba_wait(struct q6v5 *qproc, u32 status, int ms)
>
> static int q6v5proc_reset(struct q6v5 *qproc)
> {
> - u32 val;
> + u64 val;
> int ret;
> + int i;
>
> - /* Assert resets, stop core */
> - val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
> - val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
> - writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>
> - /* Enable power block headswitch, and wait for it to stabilize */
> - val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> - val |= QDSS_BHS_ON | QDSS_LDO_BYP;
> - writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> - udelay(1);
> -
> - /*
> - * Turn on memories. L2 banks should be done individually
> - * to minimize inrush current.
> - */
> - val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> - val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
> - Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
> - writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> - val |= Q6SS_L2DATA_SLP_NRET_N_2;
> - writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> - val |= Q6SS_L2DATA_SLP_NRET_N_1;
> - writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> - val |= Q6SS_L2DATA_SLP_NRET_N_0;
> - writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + if (qproc->hexagon_ver == 0x2) {
== MSS_MSM8996
> + /* Override the ACC value if required */
> + writel(QDSP6SS_ACC_OVERRIDE_VAL,
> + qproc->reg_base + QDSP6SS_STRAP_ACC);
> +
> + /* Assert resets, stop core */
> + val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
> + val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
> + writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
>
> + /* BHS require xo cbcr to be enabled */
> + val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
> + val |= 0x1;
> + writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
Please add a comment here, describing what we're waiting for (rather
than just "a magical bit 31")
> + ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
> + val, !(val & BIT(31)), 1, HALT_CHECK_MAX_LOOPS);
> + if (ret) {
> + dev_err(qproc->dev,
> + "xo cbcr enabling timed out (rc:%d)\n", ret);
> + return ret;
> + }
> + if ((val & BIT(31))) {
If bit 31 is set when the readl_poll_timeout() hits the timeout the
condition will evaluate to false and "ret" will be -ETIMEDOUT. So I
don't think this can happen.
> + dev_err(qproc->dev,
> + "Failed to enable xo branch clock.\n");
> + return -EINVAL;
> + }
> + /*
> + * Enable power block headswitch,
> + * and wait for it to stabilize
This comment fits within 80 characters, so no need to line wrap it.
> + */
> + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + val |= QDSP6v56_BHS_ON;
> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + udelay(1);
Please insert a single empty line here, to create some separation
between the "steps".
> + /* Put LDO in bypass mode */
> + val |= QDSP6v56_LDO_BYP;
> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + /*
> + * Deassert QDSP6 compiler memory clamp
> + */
> + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + val &= ~QDSP6v56_CLAMP_QMC_MEM;
> + writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
Please drop the _relaxed
> +
> + /* Deassert memory peripheral sleep and L2 memory standby */
> + val |= (Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N);
> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> + /* Turn on L1, L2, ETB and JU memories 1 at a time */
> + val = readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
> + for (i = 19; i >= 0; i--) {
> + val |= BIT(i);
> + writel(val, qproc->reg_base +
> + QDSP6SS_MEM_PWR_CTL);
> + /*
> + * Wait for 1us for both memory peripheral and
> + * data array to turn on.
> + */
/* Read back value to ensure the write is done */
And move the 1us comment below the read.
> + val |= readl(qproc->reg_base + QDSP6SS_MEM_PWR_CTL);
Please correct the indentation of this line.
> + udelay(1);
> + }
> + /* Remove word line clamp */
> + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + val &= ~QDSP6v56_CLAMP_WL;
> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + } else {
> + /* Assert resets, stop core */
> + val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
> + val |= (Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE);
> + writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
> + /*
> + * Enable power block headswitch,
> + * and wait for it to stabilize
> + */
> + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + val |= QDSS_BHS_ON | QDSS_LDO_BYP;
> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + val |= readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + udelay(1);
> +
> + /*
> + * Turn on memories. L2 banks should be done individually
> + * to minimize inrush current.
> + */
> + val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + val |= Q6SS_SLP_RET_N | Q6SS_L2TAG_SLP_NRET_N |
> + Q6SS_ETB_SLP_NRET_N | Q6SS_L2DATA_STBY_N;
> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + val |= Q6SS_L2DATA_SLP_NRET_N_2;
> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + val |= Q6SS_L2DATA_SLP_NRET_N_1;
> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + val |= Q6SS_L2DATA_SLP_NRET_N_0;
> + writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> + }
> /* Remove IO clamp */
> val &= ~Q6SS_CLAMP_IO;
> writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> @@ -664,6 +755,7 @@ static int q6v5_stop(struct rproc *rproc)
> {
> struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
> int ret;
> + int val;
>
> qproc->running = false;
>
> @@ -681,6 +773,15 @@ static int q6v5_stop(struct rproc *rproc)
> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>
> + if (qproc->hexagon_ver == 0x2) {
== MSS_MSM8996
> + /*
> + * To avoid high MX current during LPASS/MSS restart.
> + */
> + val = readl_relaxed(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
Please no _relaxed()
> + val |= Q6SS_CLAMP_IO | QDSP6v56_CLAMP_WL |
> + QDSP6v56_CLAMP_QMC_MEM;
> + writel_relaxed(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
Dito
> + }
> reset_control_assert(qproc->mss_restart);
> q6v5_clk_disable(qproc->dev, qproc->active_clks,
> qproc->active_clk_count);
Regards,
Bjorn
Powered by blists - more mailing lists