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

["Heiko Stübner" <heiko@sntech.de>]

Hi Vinod,

thanks for the review.
I've dropped all the parts that would've just gotten a "ok, changed" ;-)

Am Freitag, 14. Februar 2025, 13:13:42 MEZ schrieb Vinod Koul:
> On 13-02-25, 22:05, Heiko Stuebner wrote:

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

sadly not.

The table itself came from the vendor-kernel, and I would assume there
it came from some super-secret additional documentation Rockchip
got with the IP documentation.

It is sadly not part of the RK3588 manual.


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

Converted over to some more meaningful constants.
Did the same to bias_con0+1 below that one too.


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

It might make sense to mimic the lane_enable way of doing things, even if
just for things looking the same in both functions.

I guess disabling lanes does not really care about minimal speed differences
a switch/case would cause :-)

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

Nope ... the value is defined as bits [15:0] and both being pll lock and
stabilization timing control registers. Sadly yet again, their usage detail
is not documented, the manual even does not supply a unit for the 
register value :-(


Heiko