Sorry this slipped thru...
On 19-02-25, 23:51, Heiko Stübner wrote:
> 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
Maybe make a note that this is from vendor kernel
> 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 :-)
ok
>
> >
> > > +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 :-(
:-(
--
~Vinod
