From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@somainline.org>
Cc: agross@kernel.org, robh+dt@kernel.org, lgirdwood@gmail.com,
broonie@kernel.org, linux-arm-msm@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
phone-devel@vger.kernel.org, konrad.dybcio@somainline.org,
marijn.suijten@somainline.org, jeffrey.l.hugo@gmail.com
Subject: Re: [PATCH v5 1/3] soc: qcom: Add support for Core Power Reduction v3, v4 and Hardened
Date: Mon, 31 May 2021 18:59:34 -0500 [thread overview]
Message-ID: <YLV4ZnJ8SUbwpatl@builder.lan> (raw)
In-Reply-To: <20210121194051.484209-2-angelogioacchino.delregno@somainline.org>
On Thu 21 Jan 13:40 CST 2021, AngeloGioacchino Del Regno wrote:
> This commit introduces a new driver, based on the one for cpr v1,
> to enable support for the newer Qualcomm Core Power Reduction
> hardware, known downstream as CPR3, CPR4 and CPRh, and support
> for MSM8998 and SDM630 CPU power reduction.
>
> In these new versions of the hardware, support for various new
> features was introduced, including voltage reduction for the GPU,
> security hardening and a new way of controlling CPU DVFS,
> consisting in internal communication between microcontrollers,
> specifically the CPR-Hardened and the Operating State Manager.
>
> The CPR v3, v4 and CPRh are present in a broad range of SoCs,
> from the mid-range to the high end ones including, but not limited
> to, MSM8953/8996/8998, SDM630/636/660/845.
Why is 845 in this list? I was under the impression that CPR was dealt
with entirely by firmware starting in 845.
Also, you don't happen to have the 8996 data laying around somewhere?
> diff --git a/drivers/soc/qcom/cpr3.c b/drivers/soc/qcom/cpr3.c
[..]
> +/*
> + * cpr_get_ring_osc_factor - Get fuse corner ring oscillator factor
> + *
> + * Not all threads have different scaling factors for each
> + * Fuse Corner: if the RO factors are the same for all corners,
> + * then only one is specified, instead of uselessly repeating
> + * the same array for FC-times.
> + * This function checks for the same and gives back the right
> + * factor for the requested ring oscillator.
> + *
> + * Returns: Ring oscillator factor
This is almost kerneldoc, how about adding another '*', some parenthesis
on the function name, short description of the parameters and dropping
the 's' on Return?
> + */
> +static int cpr_get_ro_factor(const struct cpr_thread_desc *tdesc,
> + int fnum, int ro_idx)
> +{
> + int ro_fnum;
> +
> + if (tdesc->ro_avail_corners == tdesc->num_fuse_corners)
> + ro_fnum = fnum;
> + else
> + ro_fnum = 0;
> +
> + return tdesc->ro_scaling_factor[ro_fnum][ro_idx];
> +}
> +
> +static void cpr_write(struct cpr_thread *thread, u32 offset, u32 value)
> +{
> + writel_relaxed(value, thread->base + offset);
I dislike the fact that we default to the _relaxed() version of
readl/writel. Can we please switch them for non-relaxed versions, or
document why they all need to be _relaxed?
> +}
> +
> +static u32 cpr_read(struct cpr_thread *thread, u32 offset)
> +{
> + return readl_relaxed(thread->base + offset);
> +}
> +
> +static void
> +cpr_masked_write(struct cpr_thread *thread, u32 offset, u32 mask, u32 value)
> +{
> + u32 val;
> +
> + val = readl_relaxed(thread->base + offset);
> + val &= ~mask;
> + val |= value & mask;
> + writel_relaxed(val, thread->base + offset);
> +}
> +
> +static void cpr_irq_clr(struct cpr_thread *thread)
> +{
> + cpr_write(thread, CPR3_REG_IRQ_CLEAR, CPR3_IRQ_ALL);
> +}
> +
> +static void cpr_irq_clr_nack(struct cpr_thread *thread)
> +{
> + cpr_irq_clr(thread);
> + cpr_write(thread, CPR3_REG_CONT_CMD, CPR3_CONT_CMD_NACK);
> +}
> +
> +static void cpr_irq_clr_ack(struct cpr_thread *thread)
> +{
> + cpr_irq_clr(thread);
> + cpr_write(thread, CPR3_REG_CONT_CMD, CPR3_CONT_CMD_ACK);
> +}
> +
> +static void cpr_irq_set(struct cpr_thread *thread, u32 int_bits)
> +{
> + /* On CPR-hardened, interrupts are managed by and on firmware */
> + if (thread->drv->desc->cpr_type == CTRL_TYPE_CPRH)
> + return;
> +
> + cpr_write(thread, CPR3_REG_IRQ_EN, int_bits);
> +}
> +
> +/**
> + * cpr_ctl_enable - Enable CPR thread
() after the function name, here an in all kerneldoc comments below.
> + * @thread: Structure holding CPR thread-specific parameters
> + */
> +static void cpr_ctl_enable(struct cpr_thread *thread)
> +{
> + if (thread->drv->enabled && !thread->restarting)
> + cpr_masked_write(thread, CPR3_REG_CPR_CTL,
> + CPR3_CPR_CTL_LOOP_EN_MASK,
> + CPR3_CPR_CTL_LOOP_EN_MASK);
Please wrap this "block" in {}
> +}
> +
> +/**
> + * cpr_ctl_disable - Disable CPR thread
> + * @thread: Structure holding CPR thread-specific parameters
> + */
> +static void cpr_ctl_disable(struct cpr_thread *thread)
> +{
> + const struct cpr_desc *desc = thread->drv->desc;
> +
> + if (desc->cpr_type != CTRL_TYPE_CPRH) {
> + cpr_irq_set(thread, 0);
> + cpr_irq_clr(thread);
> + }
> +
> + cpr_masked_write(thread, CPR3_REG_CPR_CTL,
> + CPR3_CPR_CTL_LOOP_EN_MASK, 0);
> +}
> +
> +/**
> + * cpr_ctl_is_enabled - Check if thread is enabled
> + * @thread: Structure holding CPR thread-specific parameters
> + *
> + * Returns: true if the CPR is enabled, false if it is disabled.
It's supposed to be "Return:"
> + */
> +static bool cpr_ctl_is_enabled(struct cpr_thread *thread)
> +{
> + u32 reg_val;
> +
> + reg_val = cpr_read(thread, CPR3_REG_CPR_CTL);
> + return reg_val & CPR3_CPR_CTL_LOOP_EN_MASK;
> +}
> +
[..]
> +/**
> + * cpr_configure - Configure main HW parameters
> + * @thread: Structure holding CPR thread-specific parameters
> + *
> + * This function configures the main CPR hardware parameters, such as
> + * internal timers (and delays), sensor ownerships, activates and/or
> + * deactivates cpr-threads and others, as one sequence for all of the
> + * versions supported in this driver. By design, the function may
> + * return a success earlier if the sequence for "a previous version"
> + * has ended.
> + *
> + * NOTE: The CPR must be clocked before calling this function!
I think "Context: " is suitable for this type of comments.
> + *
> + * Returns: Zero for success or negative number on errors.
> + */
> +static int cpr_configure(struct cpr_thread *thread)
> +{
> + struct cpr_drv *drv = thread->drv;
> + const struct cpr_desc *desc = drv->desc;
> + const struct cpr_thread_desc *tdesc = thread->desc;
> + u32 val;
> + int i;
> +
> + /* Disable interrupt and CPR */
> + cpr_irq_set(thread, 0);
> + cpr_write(thread, CPR3_REG_CPR_CTL, 0);
> +
> + /* Init and save gcnt */
> + drv->gcnt = drv->ref_clk_khz * desc->gcnt_us;
> + do_div(drv->gcnt, 1000);
> +
> + /* Program the delay count for the timer */
> + val = drv->ref_clk_khz * desc->timer_delay_us;
> + do_div(val, 1000);
> + if (desc->cpr_type == CTRL_TYPE_CPR3) {
> + cpr_write(thread, CPR3_REG_CPR_TIMER_MID_CONT, val);
> +
> + val = drv->ref_clk_khz * desc->timer_updn_delay_us;
> + do_div(val, 1000);
> + cpr_write(thread, CPR3_REG_CPR_TIMER_UP_DN_CONT, val);
> + } else {
> + cpr_write(thread, CPR3_REG_CPR_TIMER_AUTO_CONT, val);
> + }
> + dev_dbg(drv->dev, "Timer count: %#0x (for %d us)\n", val,
> + desc->timer_delay_us);
> +
> + /* Program the control register */
> + val = desc->idle_clocks << CPR3_CPR_CTL_IDLE_CLOCKS_SHIFT
> + | desc->count_mode << CPR3_CPR_CTL_COUNT_MODE_SHIFT
> + | desc->count_repeat << CPR3_CPR_CTL_COUNT_REPEAT_SHIFT;
I think it's idiomatic to have the | at the end of the previous line,
rather than start with it. And below you repeat val |=, pick one style
and stick with that.
> + cpr_write(thread, CPR3_REG_CPR_CTL, val);
> +
> + /* Configure CPR default step quotients */
> + val = tdesc->step_quot_init_min << CPR3_CPR_STEP_QUOT_MIN_SHIFT
> + | tdesc->step_quot_init_max << CPR3_CPR_STEP_QUOT_MAX_SHIFT;
> +
> + cpr_write(thread, CPR3_REG_CPR_STEP_QUOT, val);
> +
> + /*
> + * Configure the CPR sensor ownership always on thread 0
> + * TODO: SDM845 has different ownership for sensors!!
> + */
> + for (i = tdesc->sensor_range_start; i < tdesc->sensor_range_end; i++)
> + cpr_write(thread, CPR3_REG_SENSOR_OWNER(i), 0);
> +
> + /* Program Consecutive Up & Down */
> + val = desc->timer_cons_up << CPR3_THRESH_CONS_UP_SHIFT;
> + val |= desc->timer_cons_down << CPR3_THRESH_CONS_DOWN_SHIFT;
> + val |= desc->up_threshold << CPR3_THRESH_UP_THRESH_SHIFT;
> + val |= desc->down_threshold << CPR3_THRESH_DOWN_THRESH_SHIFT;
> + cpr_write(thread, CPR3_REG_THRESH(tdesc->hw_tid), val);
> +
> + /* Mask all ring oscillators for all threads initially */
> + cpr_write(thread, CPR3_REG_RO_MASK(tdesc->hw_tid), CPR3_RO_MASK);
> +
> + /* HW Closed-loop control */
> + if (desc->cpr_type == CTRL_TYPE_CPR3)
Some {} here and in various places below, please
> + cpr_write(thread, CPR3_REG_HW_CLOSED_LOOP_DISABLED,
> + !desc->hw_closed_loop_en);
> + else
> + cpr_masked_write(thread, CPR4_REG_MARGIN_ADJ_CTL,
> + CPR4_MARGIN_ADJ_HW_CLOSED_LOOP_EN,
> + desc->hw_closed_loop_en ?
> + CPR4_MARGIN_ADJ_HW_CLOSED_LOOP_EN : 0);
Regards,
Bjorn
next prev parent reply other threads:[~2021-05-31 23:59 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-21 19:40 [PATCH v5 0/3] Driver for Core Power Reduction v3, v4 and Hardened AngeloGioacchino Del Regno
2021-01-21 19:40 ` [PATCH v5 1/3] soc: qcom: Add support " AngeloGioacchino Del Regno
2021-05-31 23:59 ` Bjorn Andersson [this message]
2021-06-19 12:19 ` AngeloGioacchino Del Regno
2021-01-21 19:40 ` [PATCH v5 2/3] MAINTAINERS: Add entry for Qualcomm CPRv3/v4/Hardened driver AngeloGioacchino Del Regno
2021-01-21 19:40 ` [PATCH v5 3/3] dt-bindings: soc: qcom: cpr3: Add bindings for CPR3 driver AngeloGioacchino Del Regno
2021-02-05 21:44 ` Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YLV4ZnJ8SUbwpatl@builder.lan \
--to=bjorn.andersson@linaro.org \
--cc=agross@kernel.org \
--cc=angelogioacchino.delregno@somainline.org \
--cc=broonie@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=jeffrey.l.hugo@gmail.com \
--cc=konrad.dybcio@somainline.org \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marijn.suijten@somainline.org \
--cc=phone-devel@vger.kernel.org \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).