| 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
| ||
|
Message-ID: <895b931d-15a4-4922-b52f-6c6cf7fc6243@oss.qualcomm.com>
Date: Thu, 10 Jul 2025 19:08:29 +0200
From: Konrad Dybcio <konrad.dybcio@....qualcomm.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
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 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
[...]
> +#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
Powered by blists - more mailing lists