Re: [PATCH v6 2/2] phy: rockchip: Add Samsung MIPI D-/C-PHY driver

[Vinod Koul <vkoul@kernel.org>]

On 13-02-25, 22:05, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>

> +#define PLL_CON0		0x0100
> +#define PLL_EN			BIT(12)
> +#define S_MASK			GENMASK(10, 8)
> +#define S(x)			FIELD_PREP(S_MASK, x)
> +#define P_MASK			GENMASK(5, 0)
> +#define P(x)			FIELD_PREP(P_MASK, x)
> +#define PLL_CON1		0x0104
> +#define PLL_CON2		0x0108
> +#define M_MASK			GENMASK(9, 0)
> +#define M(x)			FIELD_PREP(M_MASK, x)
> +#define PLL_CON3		0x010c

lower case, nice.

...

> +#define COMBO_MD0_GNR_CON0	0x0400
> +#define COMBO_MD0_GNR_CON1	0x0404
> +#define COMBO_MD0_ANA_CON0	0x0408
> +#define COMBO_MD0_ANA_CON1      0x040C

upper one!

> +#define COMBO_MD0_ANA_CON2	0x0410
> +
> +#define COMBO_MD0_TIME_CON0	0x0430
> +#define COMBO_MD0_TIME_CON1	0x0434
> +#define COMBO_MD0_TIME_CON2	0x0438
> +#define COMBO_MD0_TIME_CON3	0x043C

and few more, lets be lower case everywhere please?

> +	{ 200,  7,   1,  0, 33,  9,  0, 26,  5,  0, 11},
> +	{ 190,  7,   1,  0, 32,  9,  0, 25,  5,  0, 11},
> +	{ 180,  6,   1,  0, 32,  8,  0, 25,  5,  0, 10},
> +	{ 170,  6,   0,  0, 32,  8,  0, 25,  5,  0, 10},
> +	{ 160,  5,   0,  0, 31,  8,  0, 24,  4,  0,  9},
> +	{ 150,  5,   0,  0, 31,  8,  0, 24,  5,  0,  9},
> +	{ 140,  5,   0,  0, 31,  8,  0, 24,  5,  0,  8},
> +	{ 130,  4,   0,  0, 30,  6,  0, 23,  3,  0,  8},
> +	{ 120,  4,   0,  0, 30,  6,  0, 23,  3,  0,  7},
> +	{ 110,  3,   0,  0, 30,  6,  0, 23,  3,  0,  7},
> +	{ 100,  3,   0,  0, 29,  5,  0, 22,  2,  0,  6},
> +	{  90,  3,   0,  0, 29,  5,  0, 22,  2,  0,  6},
> +	{  80,  2,   0,  0, 28,  5,  0, 22,  2,  0,  5},
> +};

any word on where this table came from, maybe worth documenting that
part

> +
> +static void samsung_mipi_dcphy_bias_block_enable(struct samsung_mipi_dcphy *samsung)
> +{
> +	u32 bias_con2 = 0x3223;

magic value?

> +static void samsung_mipi_dphy_lane_disable(struct samsung_mipi_dcphy *samsung)
> +{
> +	regmap_update_bits(samsung->regmap, DPHY_MC_GNR_CON0, PHY_ENABLE, 0);
> +	regmap_update_bits(samsung->regmap, COMBO_MD0_GNR_CON0, PHY_ENABLE, 0);
> +	regmap_update_bits(samsung->regmap, COMBO_MD1_GNR_CON0, PHY_ENABLE, 0);
> +	regmap_update_bits(samsung->regmap, COMBO_MD2_GNR_CON0, PHY_ENABLE, 0);
> +	regmap_update_bits(samsung->regmap, DPHY_MD3_GNR_CON0, PHY_ENABLE, 0);

Is writing to a register (mmio) faster than a switch case for checking
lane count and disabling specific lanes?

> +static void samsung_mipi_dcphy_pll_configure(struct samsung_mipi_dcphy *samsung)
> +{
> +	regmap_update_bits(samsung->regmap, PLL_CON0, S_MASK | P_MASK,
> +			   S(samsung->pll.scaler) | P(samsung->pll.prediv));
> +
> +	if (samsung->pll.dsm < 0) {
> +		u16 dsm_tmp;
> +
> +		/* Using opposite number subtraction to find complement */
> +		dsm_tmp = abs(samsung->pll.dsm);
> +		dsm_tmp = dsm_tmp - 1;
> +		dsm_tmp ^= 0xffff;
> +		regmap_write(samsung->regmap, PLL_CON1, dsm_tmp);
> +	} else {
> +		regmap_write(samsung->regmap, PLL_CON1, samsung->pll.dsm);
> +	}
> +
> +	regmap_update_bits(samsung->regmap, PLL_CON2,
> +			   M_MASK, M(samsung->pll.fbdiv));
> +
> +	if (samsung->pll.ssc_en) {
> +		regmap_write(samsung->regmap, PLL_CON3,
> +			     MRR(samsung->pll.mrr) | MFR(samsung->pll.mfr));
> +		regmap_update_bits(samsung->regmap, PLL_CON4, SSCG_EN, SSCG_EN);
> +	}
> +
> +	regmap_write(samsung->regmap, PLL_CON5, RESET_N_SEL | PLL_ENABLE_SEL);
> +	regmap_write(samsung->regmap, PLL_CON7, PLL_LOCK_CNT(0xf000));
> +	regmap_write(samsung->regmap, PLL_CON8, PLL_STB_CNT(0xf000));

I guess you are writing to upper nibble, maybe define that, if we can

> +static __maybe_unused int samsung_mipi_dcphy_runtime_resume(struct device *dev)
> +{
> +	struct samsung_mipi_dcphy *samsung = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(samsung->pclk);
> +	if (ret) {
> +		dev_err(samsung->dev, "Failed to enable pclk, %d\n", ret);
> +		return ret;
> +	}
> +
> +	clk_prepare_enable(samsung->ref_clk);
> +	if (ret) {
> +		dev_err(samsung->dev, "Failed to enable reference clock, %d\n", ret);

No rollback of pclk here?

-- 
~Vinod