linux-kernel - Re: [PATCH v3 2/2] phy: qcom: Introduce new eDP PHY driver

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YXGvOHsJ33kJVvMk@ripper>
Date:   Thu, 21 Oct 2021 11:19:36 -0700
From:   Bjorn Andersson <bjorn.andersson@...aro.org>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     Kishon Vijay Abraham I <kishon@...com>,
        Rob Herring <robh+dt@...nel.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        abhinavk@...eaurora.org, Stephen Boyd <sboyd@...nel.org>
Subject: Re: [PATCH v3 2/2] phy: qcom: Introduce new eDP PHY driver

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

> On 16-10-21, 16:21, Bjorn Andersson wrote:
> > 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.

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

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

> > +
> > +	writel(0x1f, edp->edp + 0x58);
> 
> the register offset should be defined
> 

Yes.

> > +
> > +	msleep(20);
> > +
> > +	return 0;
> > +
[..]
> > +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?

> > +	case 5400:
> > +		step1 = 0x5c;
> > +		step2 = 0x08;
> > +		break;
> > +	default:
> > +		/* Other link rates aren't supported */
> > +		return -EINVAL;
[..]
> > +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.

> > +		return 0;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
[..]
> > +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.

[..]
> > +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