Message-ID: <8377b6bf-8823-4bcc-aee1-af17db0fbfdf@linaro.org>
Date: Fri, 11 Jul 2025 10:14:16 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
 Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>
Cc: Bryan O'Donoghue <bod@...nel.org>,
 Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
 linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
 linux-media@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

On 10/07/2025 18:08, Konrad Dybcio wrote:
> On 7/10/25 6:16 PM, Bryan O'Donoghue wrote:
>> Add a new MIPI CSI2 driver in D-PHY mode initially. The entire set of
>> existing CAMSS CSI PHY init sequences are imported in order to save time
>> and effort in later patches.
>>
>> In-line with other PHY drivers the process node name is omitted from the
>> compat string while the soc name is included.
>>
>> At the moment we follow the assignment of lane positions - the bitmap of
>> physical input lanes to logical lane numbers as a linear list per the
>> existing DPHY @lanes data-member.
>>
>> This is fine for us in upstream since we also map the lanes contiguously
>> but, our hardware can support different lane mappings so we should in the
>> future extend out the DPHY structure to capture the mapping.
>>
>> The Qualcomm 3PH class of PHYs can do both D-PHY and C-PHY mode. For now only
>> D-PHY is supported.
>>
>> In porting some of the logic over from camss-csiphy*.c to here its also
>> possible to rationalise some of the code.
>>
>> In particular use of regulator_bulk and clk_bulk as well as dropping the
>> seemingly useless and unused interrupt handler.
>>
>> The PHY sequences and a lot of the logic that goes with them are well proven
>> in CAMSS and mature so the main thing to watch out for here is how to get
>> the right sequencing of regulators, clocks and register-writes.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
>> ---
> 
> This is a good opporunity to refresh the code a bit

>> +}
>> +
>> +static irqreturn_t phy_qcom_mipi_csi2_isr(int irq, void *dev)
>> +{
>> +	const struct mipi_csi2phy_device *csi2phy = dev;
>> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
>> +	int i;
>> +
>> +	for (i = 0; i < 11; i++) {
>> +		int c = i + 22;
> 
> Please add some comments regarding these number choices
> 
>> +		u8 val = readl_relaxed(csi2phy->base +
>> +				       CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, i));
>> +
>> +		writel_relaxed(val, csi2phy->base +
>> +			       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, c));
>> +	}
>> +
>> +	writel_relaxed(0x1, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 10));
>> +	writel_relaxed(0x0, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 10));
> 
> No need for any delays?
> 
>> +
>> +	for (i = 22; i < 33; i++) {
> 
> Please provide some context here too
> 
>> +		writel_relaxed(0x0, csi2phy->base +
>> +			       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, i));
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}

TBH I think I will just drop this function in v2.

If someone comes up with a good reason for this ISR on this generation 
of hardware they are free to submit a patch to eludicate why.

During bringup/debug of this - including when I was getting RX CRC 
errors on the CSIPHY due to lacking one of two clocks, I never saw this 
ISR fire once.

I didn't enumerate the callback and you're entirely right these fixed "i 
= 22; i < 33" is to put it politely "questionable".

Just in case this code is useful on some SoC I haven't tested in the 
CAMSS source, I've left well enough alone.

Happy to drop this in the PHY driver version though.

>> +
>> +/*
>> + * phy_qcom_mipi_csi2_settle_cnt_calc - Calculate settle count value
>> + *
>> + * Helper function to calculate settle count value. This is
>> + * based on the CSI2 T_hs_settle parameter which in turn
>> + * is calculated based on the CSI2 transmitter link frequency.
>> + *
>> + * Return settle count value or 0 if the CSI2 link frequency
>> + * is not available
>> + */
>> +static u8 phy_qcom_mipi_csi2_settle_cnt_calc(s64 link_freq, u32 timer_clk_rate)
>> +{
>> +	u32 ui; /* ps */
>> +	u32 timer_period; /* ps */
>> +	u32 t_hs_prepare_max; /* ps */
>> +	u32 t_hs_settle; /* ps */
> 
> picoseconds? you can suffix the variables with _ps
> 
> Also, a reverse-Christmas-tree sorting would be neat
> 
>> +	u8 settle_cnt;
>> +
>> +	if (link_freq <= 0)
>> +		return 0;
>> +
>> +	ui = div_u64(1000000000000LL, link_freq);
> 
> PSEC_PER_SEC
> 
>> +	ui /= 2;
>> +	t_hs_prepare_max = 85000 + 6 * ui;
>> +	t_hs_settle = t_hs_prepare_max;
>> +
>> +	timer_period = div_u64(1000000000000LL, timer_clk_rate);
> 
> ditto
> 
> [...]
> 
>> +	for (i = 0; i < array_size; i++, r++) {
>> +		switch (r->mipi_csi2phy_param_type) {
>> +		case CSIPHY_SETTLE_CNT_LOWER_BYTE:
>> +			val = settle_cnt & 0xff;
>> +			break;
> 
> what about CSIPHY_SETTLE_CNT_HIGHER_BYTE?
> 
>> +		case CSIPHY_SKEW_CAL:
>> +			/* TODO: support application of skew from dt flag */
> 
> Why? What are these?

> +			continue;
>> +		case CSIPHY_DNP_PARAMS:
>> +			continue;
> 
> "do not program"? why are they in the tables then?

We try to import downstream init sequences, which themselves are 
sometimes 1:1 from the original Si testbenches unmodified, DNP_PARAMS 
come from those sequences.

I think the testbench/downstream idea here is to allow a series of 
delays and/or readback post write.

I'm certainly willing to drop anything not in the _current_ init 
sequence list.

>> +		default:
>> +			val = r->reg_data;
>> +			break;
>> +		}
>> +		writel_relaxed(val, csi2phy->base + r->reg_addr);
>> +		if (r->delay_us)
>> +			udelay(r->delay_us);
>> +	}
>> +}
>> +
>> +static bool phy_qcom_mipi_csi2_is_gen2(struct mipi_csi2phy_device *csi2phy)
>> +{
>> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
>> +
>> +	return regs->generation == GEN2;
>> +}
> 
> You only used this once, I don't think it's very useful
> 
> [...]
> 
>> +
>> +static int phy_qcom_mipi_csi2_lanes_enable(struct mipi_csi2phy_device *csi2phy,
>> +					   struct mipi_csi2phy_stream_cfg *cfg)
>> +{
>> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
>> +	struct mipi_csi2phy_lanes_cfg *lane_cfg = &cfg->lane_cfg;
>> +	u8 settle_cnt;
>> +	u8 val;
>> +	int i;
>> +
>> +	settle_cnt = phy_qcom_mipi_csi2_settle_cnt_calc(cfg->link_freq, csi2phy->timer_clk_rate);
>> +
>> +	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE;
>> +	for (i = 0; i < cfg->num_data_lanes; i++)
>> +		val |= BIT(lane_cfg->data[i].pos * 2);
>> +
>> +	writel_relaxed(val, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 5));
>> +
>> +	val = CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B;
>> +	writel_relaxed(val, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));
>> +
>> +	val = 0x02;
>> +	writel_relaxed(val, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 7));
>> +
>> +	val = 0x00;
> 
> Can we get some names for the magic (un)set bits?
> 
> [...]
> 
>> +static void
>> +phy_qcom_mipi_csi2_lanes_disable(struct mipi_csi2phy_device *csi2phy,
>> +				 struct mipi_csi2phy_stream_cfg *cfg)
>> +{
>> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
>> +
>> +	writel_relaxed(0, csi2phy->base +
>> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 5));
>> +
>> +	writel_relaxed(0, csi2phy->base +
>> +			  CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));
> 
> Does the former write need to complete before the latter?

One assumes so. All of this _relaxed() stuff is way too flaithiúlach for 
me. We tolerate it as legacy code in CAMSS but, you're right, I don't 
think its logical at all.

Dropped.

> 
>> +}
>> +
>> +static int phy_qcom_mipi_csi2_init(struct mipi_csi2phy_device *csi2phy)
>> +{
>> +	return 0;
>> +}
>> +
>> +const struct mipi_csi2phy_hw_ops phy_qcom_mipi_csi2_ops_3ph_1_0 = {
>> +	.hw_version_read = phy_qcom_mipi_csi2_hw_version_read,
>> +	.reset = phy_qcom_mipi_csi2_reset,
>> +	.lanes_enable = phy_qcom_mipi_csi2_lanes_enable,
>> +	.lanes_disable = phy_qcom_mipi_csi2_lanes_disable,
>> +	.isr = phy_qcom_mipi_csi2_isr,
>> +	.init = phy_qcom_mipi_csi2_init,
> 
> You're never calling init(), remove it. If you need it in the future,
> make it non-obligatory so that you don't need the above stub
> 

y

>> +};
>> +
>> +const struct mipi_csi2phy_clk_freq zero = { 0 };
> 
> 'zero' is a terrible global variable name

ah, I forgot to make these static.

> 
>> +
>> +const struct mipi_csi2phy_clk_freq dphy_4nm_x1e_csiphy = {
>> +	.freq = {
>> +		300000000, 400000000, 480000000
>> +	},
>> +	.num_freq = 3,
>> +};
>> +
>> +const struct mipi_csi2phy_clk_freq dphy_4nm_x1e_csiphy_timer = {
>> +	.freq = {
>> +		266666667, 400000000
>> +	},
>> +	.num_freq = 2,
>> +};
> 
> These operating points *require* different RPMh levels, let's
> grab them from an OPP table and do the ratesetting through the
> related APIs

You're right.

I'll also add regulator_set_load() in v2.

> 
>> +
>> +const struct mipi_csi2phy_soc_cfg mipi_csi2_dphy_4nm_x1e = {
>> +	.ops = &phy_qcom_mipi_csi2_ops_3ph_1_0,
>> +	.reg_info = {
>> +		.init_seq = lane_regs_x1e80100,
>> +		.lane_array_size = ARRAY_SIZE(lane_regs_x1e80100),
>> +		.offset = 0x1000,
> 
> common_regs_offset
> 
>> +		.generation = GEN2,
>> +	},
>> +	.supply_names = (const char *[]){
>> +		"vdda-0p8",
>> +		"vdda-1p2"
>> +	},
>> +	.num_supplies = 2,
>> +	.clk_names = (const char *[]) {
>> +		"camnoc_axi",
>> +		"cpas_ahb",
>> +		"csiphy",
>> +		"csiphy_timer"
>> +	},
>> +	.num_clk = 4,
>> +	.clk_freq = {
>> +		zero,
>> +		zero,
>> +		dphy_4nm_x1e_csiphy,
>> +		dphy_4nm_x1e_csiphy_timer,
> 
> Throw the ratesetting clocks into opp_config and grab the rest
> through clk_bulk_get_all()
> 
>> +	},
>> +};
>> diff --git a/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..1def2d1258d9dd3835c09c6804e08dfffc184248
>> --- /dev/null
>> +++ b/drivers/phy/qualcomm/phy-qcom-mipi-csi2-core.c
>> @@ -0,0 +1,281 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2025, Linaro Ltd.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/phy/phy-mipi-dphy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/reset.h>
>> +#include <linux/slab.h>
>> +
>> +#include "phy-qcom-mipi-csi2.h"
>> +
>> +#define CAMSS_CLOCK_MARGIN_NUMERATOR 105
>> +#define CAMSS_CLOCK_MARGIN_DENOMINATOR 100
>> +
>> +static inline void phy_qcom_mipi_csi2_add_clock_margin(u64 *rate)
>> +{
>> +	*rate *= CAMSS_CLOCK_MARGIN_NUMERATOR;
>> +	*rate = div_u64(*rate, CAMSS_CLOCK_MARGIN_DENOMINATOR);
>> +}
> 
> I can't find downstream doing that

Inherited from CAMSS where it really does something useful.

I'm relucant to change this... I'll try it though.

> 
> [...]
> 
>> +	/*
>> +	 * phy_configure_opts_mipi_dphy.lanes starts from zero to
>> +	 * the maximum number of enabled lanes.
>> +	 *
>> +	 * TODO: add support for bitmask of enabled lanes and polarities
>> +	 * of those lanes to the phy_configure_opts_mipi_dphy struct.
>> +	 * For now take the polarities as zero and the position as fixed
>> +	 * this is fine as no current upstream implementation maps otherwise.
>> +	 */
> 
> Can we at least grab the data and make sure it matches the
> default hw configuration now, so that we won't break bad
> DTs in the future?

Hmm. I'll have a think about that.

---
bod

Powered by blists - more mailing lists