Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
In-Reply-To: <20210511041930.592483-2-bjorn.andersson@linaro.org>
References: <20210511041930.592483-1-bjorn.andersson@linaro.org> <20210511041930.592483-2-bjorn.andersson@linaro.org>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.9.1
Date:   Mon, 10 May 2021 22:22:21 -0700
Message-ID: <CAE-0n50qWuny_1oYEMSZ+cfmCvnumk_UTPxUvZ-3wWdgOCioNA@mail.gmail.com>
Subject: Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver
To:     Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Rob Clark <robdclark@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Vinod Koul <vkoul@kernel.org>, sbillaka@codeaurora.org
Cc:     linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Bjorn Andersson (2021-05-10 21:19:30)
> Many recent Qualcomm platforms comes with native DP and eDP support.
> This consists of a controller int he 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.

Thank you for not slamming it into the same mega driver.

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

Can we make some library code for "DP" stuff in qmp that the two can
call to share the logic?

>
> The eDP PHY is very similar to the dedicated (non-USB) DP PHY, but only
> the prior is supported for now.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/phy/qualcomm/Kconfig        |  10 +
>  drivers/phy/qualcomm/Makefile       |   1 +
>  drivers/phy/qualcomm/phy-qcom-edp.c | 750 ++++++++++++++++++++++++++++
>  3 files changed, 761 insertions(+)
>  create mode 100644 drivers/phy/qualcomm/phy-qcom-edp.c
>
> diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig
> index 7f6fcb8ec5ba..13ae6f4f3112 100644
> --- a/drivers/phy/qualcomm/Kconfig
> +++ b/drivers/phy/qualcomm/Kconfig
> @@ -114,3 +114,13 @@ config PHY_QCOM_IPQ806X_USB
>           This option enables support for the Synopsis PHYs present inside the
>           Qualcomm USB3.0 DWC3 controller on ipq806x SoC. This driver supports
>           both HS and SS PHY controllers.
> +
> +config PHY_QCOM_EDP

Can this come somewhere alphabet sorted?

> +       tristate "Qualcomm eDP PHY driver"
> +       depends on ARCH_QCOM || COMPILE_TEST

I wonder if this could be pulled up above for the whole file.

> +       depends on OF
> +       depends on COMMON_CLK
> +       select GENERIC_PHY
> +       help
> +         Enable this driver to support the Qualcomm eDP PHY found in various
> +         Qualcomm chipsets.
> diff --git a/drivers/phy/qualcomm/Makefile b/drivers/phy/qualcomm/Makefile
> index 47acbd7daa3a..1afd0882dbcc 100644
> --- a/drivers/phy/qualcomm/Makefile
> +++ b/drivers/phy/qualcomm/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_PHY_QCOM_USB_HS_28NM)    += phy-qcom-usb-hs-28nm.o
>  obj-$(CONFIG_PHY_QCOM_USB_SS)          += phy-qcom-usb-ss.o
>  obj-$(CONFIG_PHY_QCOM_USB_SNPS_FEMTO_V2)+= phy-qcom-snps-femto-v2.o
>  obj-$(CONFIG_PHY_QCOM_IPQ806X_USB)             += phy-qcom-ipq806x-usb.o
> +obj-$(CONFIG_PHY_QCOM_EDP)             += phy-qcom-edp.o

Also alphabet sorted instead of tacked onto the end?

> diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
> new file mode 100644
> index 000000000000..ff0508e49548
> --- /dev/null
> +++ b/drivers/phy/qualcomm/phy-qcom-edp.c
> @@ -0,0 +1,750 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2017, 2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2021, Linaro Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#include <dt-bindings/phy/phy.h>
> +
> +#define DP_PHY_PD_CTL_PWRDN                    0x001
> +#define DP_PHY_PD_CTL_PSR_PWRDN                        0x002
> +#define DP_PHY_PD_CTL_AUX_PWRDN                        0x004
> +#define DP_PHY_PD_CTL_LANE_0_1_PWRDN           0x008
> +#define DP_PHY_PD_CTL_LANE_2_3_PWRDN           0x010
> +#define DP_PHY_PD_CTL_PLL_PWRDN                        0x020
> +#define DP_PHY_PD_CTL_DP_CLAMP_EN              0x040
> +
> +#define DP_PHY_CFG                              0x0010
> +#define DP_PHY_CFG_1                            0x0014
> +#define DP_PHY_PD_CTL                           0x001C
> +#define DP_PHY_MODE                             0x0020
> +
> +#define DP_PHY_AUX_CFG0                                0x0020
> +#define DP_PHY_AUX_CFG1                                0x0024
> +#define DP_PHY_AUX_CFG2                                0x0028
> +#define DP_PHY_AUX_CFG3                                0x002c
> +#define DP_PHY_AUX_CFG4                                0x0030
> +#define DP_PHY_AUX_CFG5                                0x0034
> +#define DP_PHY_AUX_CFG6                                0x0038
> +#define DP_PHY_AUX_CFG7                                0x003c
> +#define DP_PHY_AUX_CFG8                                0x0040
> +#define DP_PHY_AUX_CFG9                                0x0044
> +
> +#define DP_PHY_VCO_DIV                          0x0074
> +#define DP_PHY_TX0_TX1_LANE_CTL                 0x007C
> +#define DP_PHY_TX2_TX3_LANE_CTL                 0x00A0
> +
> +#define DP_PHY_SPARE0                           0x00CC
> +#define DP_PHY_STATUS                           0x00E0
> +
> +/* Tx registers */
> +#define TXn_CLKBUF_ENABLE                       0x0000
> +#define TXn_TX_EMP_POST1_LVL                    0x0004
> +
> +#define TXn_TX_DRV_LVL                          0x0014
> +#define TXn_TX_DRV_LVL_OFFSET                   0x0018
> +#define TXn_RESET_TSYNC_EN                      0x001C
> +//#define TXn_PRE_STALL_LDO_BOOST_EN            0x0020
> +#define TXn_LDO_CONFIG                          0x0084
> +#define TXn_TX_BAND                             0x0028
> +#define TXn_INTERFACE_SELECT                    0x0024
> +
> +#define TXn_RES_CODE_LANE_OFFSET_TX0            0x0044
> +#define TXn_RES_CODE_LANE_OFFSET_TX1            0x0048
> +//#define TXn_RES_CODE_LANE_OFFSET_RX           0x0040
> +
> +#define TXn_TRANSCEIVER_BIAS_EN                 0x0054
> +#define TXn_HIGHZ_DRVR_EN                       0x0058
> +#define TXn_TX_POL_INV                          0x005C
> +#define TXn_PARRATE_REC_DETECT_IDLE_EN          0x0060
> +#define TXn_LANE_MODE_1                         0x0064
> +
> +#define TXn_TRAN_DRVR_EMP_EN                    0x0078
> +//#define TXn_TX_INTERFACE_MODE                 0x00BC

Future work? Maybe add a TODO?

> +
> +#define TXn_VMODE_CTRL1                         0x007C
> +
> +/* PLL register offset */
> +#define QSERDES_COM_BG_TIMER                    0x000C
> +#define QSERDES_COM_BIAS_EN_CLKBUFLR_EN         0x0044
> +#define QSERDES_COM_CLK_ENABLE1                 0x0048
> +#define QSERDES_COM_SYS_CLK_CTRL                0x004C
[..]
> +static int qcom_edp_clks_register(struct qcom_edp *edp, struct device_node *np)
> +{
> +       struct clk_init_data init = { };
> +       int ret;
> +
> +       init.ops = &qcom_edp_dp_link_clk_ops;
> +       init.name = "edp_phy_pll_link_clk";
> +       edp->dp_link_hw.init = &init;
> +       ret = devm_clk_hw_register(edp->dev, &edp->dp_link_hw);
> +       if (ret)
> +               return ret;
> +
> +       init.ops = &qcom_edp_dp_pixel_clk_ops;
> +       init.name = "edp_phy_pll_vco_div_clk";
> +       edp->dp_pixel_hw.init = &init;
> +       ret = devm_clk_hw_register(edp->dev, &edp->dp_pixel_hw);
> +       if (ret)
> +               return ret;
> +
> +       ret = of_clk_add_hw_provider(np, qcom_edp_dp_clks_hw_get, edp);
> +       if (ret)
> +               return ret;
> +
> +       /*
> +        * Roll a devm action because the clock provider is the child node, but

Is it still a child node?

> +        * the child node is not actually a device.
> +        */
> +       ret = devm_add_action(edp->dev, phy_clk_release_provider, np);
> +       if (ret)
> +               phy_clk_release_provider(np);
> +
> +       return ret;
> +}
> +