On Thu 21 Oct 10:40 PDT 2021, Vinod Koul wrote:

On 16-10-21, 16:21, Bjorn Andersson wrote:

quoted

Many recent Qualcomm platforms comes with native DP and eDP support.
This consists of a controller in the MDSS and a QMP-like PHY.

While similar to the well known QMP block, the eDP PHY only has TX lanes
and the programming sequences are slightly different. Rather than
continuing the trend of parameterize the QMP driver to pieces, this
introduces the support as a new driver.

The registration of link and pixel clocks are borrowed from the QMP
driver. The non-DP link frequencies are omitted for now.

The eDP PHY is very similar to the dedicated (non-USB) DP PHY, but only
the prior is supported for now.

since this is QMP phy, pls add an explanation why common QMP driver
is not used here?

Will do.

quoted

+static int qcom_edp_phy_init(struct phy *phy)
+{
+	struct qcom_edp *edp = phy_get_drvdata(phy);
+	int ret;
+
+	ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies);
+	if (ret)
+		return ret;
+
+	ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks);
+	if (ret)
+		goto out_disable_supplies;
+
+	writel(DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN |
+	       DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN,
+	       edp->edp + DP_PHY_PD_CTL);
+
+	writel(0x17, edp->pll + QSERDES_V4_COM_BIAS_EN_CLKBUFLR_EN);

magic?

I'll see if I can figure out what this magic number represents.

quoted

+
+	writel(DP_PHY_PD_CTL_PSR_PWRDN, edp->edp + DP_PHY_PD_CTL);
+	msleep(20);
+
+	writel(DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN |
+	       DP_PHY_PD_CTL_LANE_0_1_PWRDN | DP_PHY_PD_CTL_LANE_2_3_PWRDN |
+	       DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN,
+	       edp->edp + DP_PHY_PD_CTL);
+
+	writel(0x00, edp->edp + DP_PHY_AUX_CFG0);
+	writel(0x13, edp->edp + DP_PHY_AUX_CFG1);
+	writel(0x24, edp->edp + DP_PHY_AUX_CFG2);
+	writel(0x00, edp->edp + DP_PHY_AUX_CFG3);
+	writel(0x0a, edp->edp + DP_PHY_AUX_CFG4);
+	writel(0x26, edp->edp + DP_PHY_AUX_CFG5);
+	writel(0x0a, edp->edp + DP_PHY_AUX_CFG6);
+	writel(0x03, edp->edp + DP_PHY_AUX_CFG7);
+	writel(0x37, edp->edp + DP_PHY_AUX_CFG8);
+	writel(0x03, edp->edp + DP_PHY_AUX_CFG9);

In qmp phy we use a table for this, that looks very elegant and I am
sure next rev will have different magic numbers, so should we go the
table approach here on as well..?

Yes, these numbers are different for DP, so that makes sense.

quoted

+
+	writel(0x1f, edp->edp + 0x58);

the register offset should be defined

Yes.

quoted

+
+	msleep(20);
+
+	return 0;
+

[..]

quoted

+static int qcom_edp_configure_ssc(const struct qcom_edp *edp)
+{
+	const struct phy_configure_opts_dp *dp_opts = &edp->dp_opts;
+	u32 step1;
+	u32 step2;
+
+	switch (dp_opts->link_rate) {
+	case 1620:
+	case 2700:
+	case 8100:
+		step1 = 0x45;
+		step2 = 0x06;
+		break;

line after each break please (here & few other places)

You mean an empty line between the break and the next case? That doesn't
seem standard?

quoted

+	case 5400:
+		step1 = 0x5c;
+		step2 = 0x08;
+		break;
+	default:
+		/* Other link rates aren't supported */
+		return -EINVAL;

[..]

quoted

+static int qcom_edp_dp_pixel_clk_determine_rate(struct clk_hw *hw,
+						struct clk_rate_request *req)
+{
+	switch (req->rate) {
+	case 1620000000UL / 2:
+	case 2700000000UL / 2:
+	/* 5.4 and 8.1 GHz are same link rate as 2.7GHz, i.e. div 4 and div 6 */

above rates are 1.62 and 2.7, where is 5.4 and 8.1... what am i missing?

As the comments says 2.7, 5.4 and 8.1 all has req->rate of 1350000000,
with different dividers. But we're not allowed to "document" that by
listing 2.7/2, 5.4/4 and 8.1/6 in the switch statement.

quoted

+		return 0;
+	default:
+		return -EINVAL;
+	}
+}

[..]

quoted

+static const struct clk_ops qcom_edp_dp_pixel_clk_ops = {
+	.determine_rate = qcom_edp_dp_pixel_clk_determine_rate,
+	.recalc_rate = qcom_edp_dp_pixel_clk_recalc_rate,
+};
+
+static int qcom_edp_dp_link_clk_determine_rate(struct clk_hw *hw,
+					       struct clk_rate_request *req)

maybe is rate_valid/supported be better name for this?

It's named per the clk_ops.

[..]

quoted

+static const struct clk_ops qcom_edp_dp_link_clk_ops = {
+	.determine_rate = qcom_edp_dp_link_clk_determine_rate,
+	.recalc_rate = qcom_edp_dp_link_clk_recalc_rate,
+};

[..]

Thanks,
Bjorn

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy