Re: [PATCH 2/2] phy: qcom-mipi-csi2: Add a CSI2 MIPI D-PHY driver

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

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@linaro.org>
> ---

This is a good opporunity to refresh the code a bit

[...]

> +#define CSIPHY_3PH_LNn_CFG1(n)				(0x000 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG1_SWI_REC_DLY_PRG		(BIT(7) | BIT(6))
> +#define CSIPHY_3PH_LNn_CFG2(n)				(0x004 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG2_LP_REC_EN_INT		BIT(3)
> +#define CSIPHY_3PH_LNn_CFG3(n)				(0x008 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG4(n)				(0x00c + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS		0xa4
> +#define CSIPHY_3PH_LNn_CFG4_T_HS_CLK_MISS_660		0xa5
> +#define CSIPHY_3PH_LNn_CFG5(n)				(0x010 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG5_T_HS_DTERM			0x02
> +#define CSIPHY_3PH_LNn_CFG5_HS_REC_EQ_FQ_INT		0x50
> +#define CSIPHY_3PH_LNn_TEST_IMP(n)			(0x01c + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_TEST_IMP_HS_TERM_IMP		0xa
> +#define CSIPHY_3PH_LNn_MISC1(n)				(0x028 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_MISC1_IS_CLKLANE			BIT(2)
> +#define CSIPHY_3PH_LNn_CFG6(n)				(0x02c + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG6_SWI_FORCE_INIT_EXIT		BIT(0)
> +#define CSIPHY_3PH_LNn_CFG7(n)				(0x030 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG7_SWI_T_INIT			0x2
> +#define CSIPHY_3PH_LNn_CFG8(n)				(0x034 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG8_SWI_SKIP_WAKEUP		BIT(0)
> +#define CSIPHY_3PH_LNn_CFG8_SKEW_FILTER_ENABLE		BIT(1)
> +#define CSIPHY_3PH_LNn_CFG9(n)				(0x038 + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CFG9_SWI_T_WAKEUP		0x1
> +#define CSIPHY_3PH_LNn_CSI_LANE_CTRL15(n)		(0x03c + 0x100 * (n))
> +#define CSIPHY_3PH_LNn_CSI_LANE_CTRL15_SWI_SOT_SYMBOL	0xb8

> +
> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(offset, n)	((offset) + 0x4 * (n))
> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL5_CLK_ENABLE	BIT(7)
> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_COMMON_PWRDN_B	BIT(0)
> +#define CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID	BIT(1)
> +#define CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(offset, n)	((offset) + 0xb0 + 0x4 * (n))> +
> +#define CSIPHY_DEFAULT_PARAMS				0
> +#define CSIPHY_LANE_ENABLE				1
> +#define CSIPHY_SETTLE_CNT_LOWER_BYTE			2
> +#define CSIPHY_SETTLE_CNT_HIGHER_BYTE			3
> +#define CSIPHY_DNP_PARAMS				4
> +#define CSIPHY_2PH_REGS					5
> +#define CSIPHY_3PH_REGS					6
> +#define CSIPHY_SKEW_CAL					7

These could be turned into separate sub-sequences instead (like e.g. in
qmpphy drivers)

Specifically with 2PH_REGS/3PH_REGS, I'm assuming 2PH refers to another
piece of hardware which could have its own init sequences..

> +
> +/* 4nm 2PH v 2.1.2 2p5Gbps 4 lane DPHY mode */
> +static const struct
> +mipi_csi2phy_lane_regs lane_regs_x1e80100[] = {
> +	/* Power up lanes 2ph mode */
> +	{0x1014, 0xD5, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	{0x101C, 0x7A, 0x00, CSIPHY_DEFAULT_PARAMS},
> +	{0x1018, 0x01, 0x00, CSIPHY_DEFAULT_PARAMS},

struct mipi_csi2phy_lane_regs {
	const s32 reg_addr;
	const s32 reg_data;
	const u32 delay_us;
	const u32 mipi_csi2phy_param_type;
};

let's use designated initializers, decimal for delay_us (omit it
where unnecessary)

[...]

> +static inline const struct mipi_csi2phy_device_regs *
> +csi2phy_dev_to_regs(const struct mipi_csi2phy_device *csi2phy)
> +{
> +	return &csi2phy->soc_cfg->reg_info;
> +}

#define?

> +
> +static void phy_qcom_mipi_csi2_hw_version_read(struct mipi_csi2phy_device *csi2phy)
> +{
> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
> +	u32 hw_version;
> +
> +	writel(CSIPHY_3PH_CMN_CSI_COMMON_CTRL6_SHOW_REV_ID, csi2phy->base +
> +	       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 6));

Do we need to clear this later?> +
> +	hw_version = readl_relaxed(csi2phy->base +
> +				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 12));
> +	hw_version |= readl_relaxed(csi2phy->base +
> +				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 13)) << 8;
> +	hw_version |= readl_relaxed(csi2phy->base +
> +				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 14)) << 16;
> +	hw_version |= readl_relaxed(csi2phy->base +
> +				   CSIPHY_3PH_CMN_CSI_COMMON_STATUSn(regs->offset, 15)) << 24;

I don't like how you're ORing the entirety of these 32-bit registers
into the saved value, maybe we can get the lower 8 bits of each and
construct the result with more guardrails

> +
> +	csi2phy->hw_version = hw_version;
> +
> +	dev_dbg(csi2phy->dev, "CSIPHY 3PH HW Version = 0x%08x\n", hw_version);

dev_dbg_once() will suffice

> +}
> +
> +/*
> + * phy_qcom_mipi_csi2_reset - Perform software reset on CSIPHY module
> + * @phy_qcom_mipi_csi2: CSIPHY device
> + */
> +static void phy_qcom_mipi_csi2_reset(struct mipi_csi2phy_device *csi2phy)
> +{
> +	const struct mipi_csi2phy_device_regs *regs = csi2phy_dev_to_regs(csi2phy);
> +
> +	writel_relaxed(0x1, csi2phy->base +
> +		      CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 0));
> +	usleep_range(5000, 8000);
> +	writel_relaxed(0x0, csi2phy->base +
> +		       CSIPHY_3PH_CMN_CSI_COMMON_CTRLn(regs->offset, 0));

Can we get a name for this BIT(0)?

> +}
> +
> +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;
> +}
> +
> +/*
> + * 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?

> +		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?

> +}
> +
> +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

> +};
> +
> +const struct mipi_csi2phy_clk_freq zero = { 0 };

'zero' is a terrible global variable name

> +
> +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

> +
> +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

[...]

> +	/*
> +	 * 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?

[...]


> +static int phy_qcom_mipi_csi2_power_off(struct phy *phy)
> +{
> +	struct mipi_csi2phy_device *csi2phy = phy_get_drvdata(phy);
> +
> +	clk_bulk_disable_unprepare(csi2phy->soc_cfg->num_clk,
> +				   csi2phy->clks);
> +	regulator_bulk_disable(csi2phy->soc_cfg->num_supplies,
> +			       csi2phy->supplies);

downstream issues a sw reset across a number of registers first

[...]> +static const struct of_device_id phy_qcom_mipi_csi2_of_match_table[] = {
> +	{ .compatible	= "qcom,x1e80100-mipi-csi2-combo-phy", .data = &mipi_csi2_dphy_4nm_x1e },

odd >1 space

Konrad