Re: [PATCH v5 1/3] soc: qcom: Add support for Core Power Reduction v3, v4 and Hardened

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