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@xxxxxxxxxx>
> ---
> 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
