Re: [PATCH v4 3/6] phy: qcom-qmp-pcie: support separate tables for EP mode

From: Johan Hovold <johan@kernel.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: "Andy Gross" <agross@kernel.org>,
	"Bjorn Andersson" <andersson@kernel.org>,
	"Konrad Dybcio" <konrad.dybcio@somainline.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Vinod Koul" <vkoul@kernel.org>,
	"Kishon Vijay Abraham I" <kishon@ti.com>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-phy@lists.infradead.org
Subject: Re: [PATCH v4 3/6] phy: qcom-qmp-pcie: support separate tables for EP mode
Date: Mon, 26 Sep 2022 09:48:00 +0200	[thread overview]
Message-ID: <YzFZMGezfg26T9r5@hovoldconsulting.com> (raw)
In-Reply-To: <20220924160302.285875-4-dmitry.baryshkov@linaro.org>

On Sat, Sep 24, 2022 at 07:02:59PM +0300, Dmitry Baryshkov wrote:
> The PCIe QMP PHY requires different programming sequences when being
> used for the RC (Root Complex) or for the EP (End Point) modes. Allow
> selecting the submode and thus selecting a set of PHY programming
> tables.
> 
> Since the RC and EP modes share common some common init sequence, the
> common sequence is kept in the main table and the sequence differences
> are pushed to the extra tables.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 67 ++++++++++++++++++++----
>  1 file changed, 58 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index 6e8c74585670..1fc23df59454 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -1320,10 +1320,14 @@ struct qmp_phy_cfg {
>  	/* Main init sequence for PHY blocks - serdes, tx, rx, pcs */
>  	struct qmp_phy_cfg_tables common;
>  	/*
> -	 * Additional init sequence for PHY blocks, providing additional
> -	 * register programming. Unless required it can be left omitted.
> +	 * Additional init sequences for PHY blocks, providing additional
> +	 * register programming. They are used for providing separate sequences
> +	 * for the Root Complex and for the End Point usecases.
> +	 *
> +	 * If EP mode is not supported, both tables can be left empty.
>  	 */
> -	struct qmp_phy_cfg_tables *extra;
> +	struct qmp_phy_cfg_tables *extra_rc; /* for the RC only */
> +	struct qmp_phy_cfg_tables *extra_ep; /* for the EP only */

So with the additional "_rc" and "_ep" suffixes this would be more
obvious as for example:

	struct qmp_phy_cfg_tables tbls_common;
	struct qmp_phy_cfg_tables tbls_rc;
	struct qmp_phy_cfg_tables tbls_ep;

The "for the RC only" comments doesn't really add anything.

>  
>  	/* clock ids to be requested */
>  	const char * const *clk_list;
> @@ -1367,6 +1371,7 @@ struct qmp_phy_cfg {
>   * @pcs_misc: iomapped memory space for lane's pcs_misc
>   * @pipe_clk: pipe clock
>   * @qmp: QMP phy to which this lane belongs
> + * @extra: currently selected PHY extra init table set

"extra" is not a descriptive name here either.

>   */
>  struct qmp_phy {
>  	struct phy *phy;
> @@ -1379,6 +1384,7 @@ struct qmp_phy {
>  	void __iomem *rx2;
>  	void __iomem *pcs_misc;
>  	struct clk *pipe_clk;
> +	const struct qmp_phy_cfg_tables *extra;
>  	struct qcom_qmp *qmp;
>  };
>  
> @@ -1624,7 +1630,15 @@ static const struct qmp_phy_cfg sm8250_qmp_gen3x1_pciephy_cfg = {
>  	.pcs_misc_tbl		= sm8250_qmp_pcie_pcs_misc_tbl,
>  	.pcs_misc_tbl_num	= ARRAY_SIZE(sm8250_qmp_pcie_pcs_misc_tbl),
>  	},
> -	.extra = &(struct qmp_phy_cfg_tables) {
> +	/*
> +	 * For sm8250 the split between the primary and extra_rc tables is
> +	 * historical, it reflects the programming sequence common to all PCIe
> +	 * PHYs on this platform and a sequence required for this particular
> +	 * PHY type. If EP support for sm8250 is required, the
> +	 * primary/extra_rc split is to be reconsidered and adjusted
> +	 * according to EP programming sequence.
> +	 */

Not sure this is needed either. I'm currently using the "_sec" tables
(future "extra") for sc8280xp as well to handle minor variations in init
sequences between the different PHY types.

> +	.extra_rc = &(struct qmp_phy_cfg_tables) {
>  	.serdes_tbl		= sm8250_qmp_gen3x1_pcie_serdes_tbl,
>  	.serdes_tbl_num		= ARRAY_SIZE(sm8250_qmp_gen3x1_pcie_serdes_tbl),
>  	.rx_tbl			= sm8250_qmp_gen3x1_pcie_rx_tbl,

> @@ -2000,8 +2022,12 @@ static int qmp_pcie_power_on(struct phy *phy)
>  	unsigned int mask, val, ready;
>  	int ret;
>  
> +	/* Default to RC mode if the mode was not selected using phy_set_mode_ext() */
> +	if (!qphy->extra)
> +		qphy->extra = cfg->extra_rc;

It would probably be better to just store the submode in
phy_set_mode_ext() and pick the right table here.

That way you don't need to describe what's really going on in a comment
as it would be apparent from the code.

> +
>  	qmp_pcie_serdes_init(qphy, &cfg->common);
> -	qmp_pcie_serdes_init(qphy, cfg->extra);
> +	qmp_pcie_serdes_init(qphy, qphy->extra);
>  
>  	ret = clk_prepare_enable(qphy->pipe_clk);
>  	if (ret) {

> +static int qmp_pcie_set_mode(struct phy *phy,

Do you really need a line break?

> +				 enum phy_mode mode, int submode)
> +{
> +	struct qmp_phy *qphy = phy_get_drvdata(phy);
> +
> +	switch (submode) {
> +	case PHY_MODE_PCIE_RC:
> +		qphy->extra = qphy->cfg->extra_rc;
> +		break;
> +	case PHY_MODE_PCIE_EP:
> +		qphy->extra = qphy->cfg->extra_ep;
> +		break;
> +	default:
> +		dev_err(&phy->dev, "Unuspported submode %d\n", submode);

Typo: unsupported

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int qmp_pcie_vreg_init(struct device *dev, const struct qmp_phy_cfg *cfg)
>  {
>  	struct qcom_qmp *qmp = dev_get_drvdata(dev);
> @@ -2224,6 +2270,7 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
>  static const struct phy_ops qmp_pcie_ops = {
>  	.power_on	= qmp_pcie_enable,
>  	.power_off	= qmp_pcie_disable,
> +	.set_mode	= qmp_pcie_set_mode,
>  	.owner		= THIS_MODULE,
>  };
>  
> @@ -2278,7 +2325,9 @@ static int qmp_pcie_create(struct device *dev, struct device_node *np, int id,
>  		qphy->pcs_misc = qphy->pcs + 0x400;
>  
>  	if (IS_ERR(qphy->pcs_misc)) {
> -		if (cfg->common.pcs_misc_tbl || cfg->extra->pcs_misc_tbl)
> +		if (cfg->common.pcs_misc_tbl ||
> +		    cfg->extra_rc->pcs_misc_tbl ||
> +		    cfg->extra_ep->pcs_misc_tbl)

I already pointed out the NULL deref here.

>  			return PTR_ERR(qphy->pcs_misc);
>  	}

Johan

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