* Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver
2021-05-11 4:19 ` [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver Bjorn Andersson
@ 2021-05-11 5:22 ` Stephen Boyd
2021-05-14 10:38 ` Vinod Koul
2021-05-11 14:24 ` Jeffrey Hugo
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2021-05-11 5:22 UTC (permalink / raw)
To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov,
Kishon Vijay Abraham I, Rob Clark, Rob Herring, Vinod Koul,
sbillaka
Cc: linux-arm-msm, linux-phy, devicetree, linux-kernel
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;
> +}
> +
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver
2021-05-11 5:22 ` Stephen Boyd
@ 2021-05-14 10:38 ` Vinod Koul
0 siblings, 0 replies; 12+ messages in thread
From: Vinod Koul @ 2021-05-14 10:38 UTC (permalink / raw)
To: Stephen Boyd
Cc: Andy Gross, Bjorn Andersson, Dmitry Baryshkov,
Kishon Vijay Abraham I, Rob Clark, Rob Herring, sbillaka,
linux-arm-msm, linux-phy, devicetree, linux-kernel
On 10-05-21, 22:22, Stephen Boyd wrote:
> 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?
I think we should split QMP into a library of common code which the
respective function driver DP, UFS, USB QMP drivers (re)use across...
--
~Vinod
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver
2021-05-11 4:19 ` [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver Bjorn Andersson
2021-05-11 5:22 ` Stephen Boyd
@ 2021-05-11 14:24 ` Jeffrey Hugo
2021-05-11 14:46 ` Dmitry Baryshkov
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Jeffrey Hugo @ 2021-05-11 14:24 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Kishon Vijay Abraham I, Vinod Koul, Rob Herring,
sbillaka, Rob Clark, Dmitry Baryshkov, Stephen Boyd, MSM,
linux-phy, DTML, lkml
On Mon, May 10, 2021 at 10:21 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> Many recent Qualcomm platforms comes with native DP and eDP support.
> This consists of a controller int he MDSS and a QMP-like PHY.
"in the"
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver
2021-05-11 4:19 ` [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver Bjorn Andersson
2021-05-11 5:22 ` Stephen Boyd
2021-05-11 14:24 ` Jeffrey Hugo
@ 2021-05-11 14:46 ` Dmitry Baryshkov
2021-05-14 10:40 ` Vinod Koul
2021-08-10 3:15 ` sbillaka
4 siblings, 0 replies; 12+ messages in thread
From: Dmitry Baryshkov @ 2021-05-11 14:46 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Kishon Vijay Abraham I, Vinod Koul, Rob Herring,
Sankeerth Billakanti, Rob Clark, Stephen Boyd,
open list:DRM DRIVER FOR MSM ADRENO GPU, linux-phy,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
open list
On Tue, 11 May 2021 at 07:19, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> 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.
>
> 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.
>
> 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.
> +
Just a generic suggestion: what about naming it
PHY_QCOM_QMP_EDP/phy-qcom-qmp-edp.c, so that it stays close to the
"big" qmp driver and also so that other qmp parts can follow this
example if we ever split the bg driver in future?
> +config PHY_QCOM_EDP
> + tristate "Qualcomm eDP PHY driver"
> + depends on ARCH_QCOM || COMPILE_TEST
> + 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
> 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
> +
> +#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
> +#define QSERDES_COM_SYSCLK_BUF_ENABLE 0x0050
> +#define QSERDES_COM_PLL_IVCO 0x0058
> +
> +#define QSERDES_COM_CP_CTRL_MODE0 0x0074
> +#define QSERDES_COM_PLL_RCTRL_MODE0 0x007C
> +#define QSERDES_COM_PLL_CCTRL_MODE0 0x0084
> +#define QSERDES_COM_SYSCLK_EN_SEL 0x0094
> +#define QSERDES_COM_RESETSM_CNTRL 0x009C
> +#define QSERDES_COM_LOCK_CMP_EN 0x00A4
> +#define QSERDES_COM_LOCK_CMP1_MODE0 0x00AC
> +#define QSERDES_COM_LOCK_CMP2_MODE0 0x00B0
> +
> +#define QSERDES_COM_DEC_START_MODE0 0x00BC
> +#define QSERDES_COM_DIV_FRAC_START1_MODE0 0x00CC
> +#define QSERDES_COM_DIV_FRAC_START2_MODE0 0x00D0
> +#define QSERDES_COM_DIV_FRAC_START3_MODE0 0x00D4
> +#define QSERDES_COM_INTEGLOOP_GAIN0_MODE0 0x00EC
> +#define QSERDES_COM_INTEGLOOP_GAIN1_MODE0 0x00F0
> +#define QSERDES_COM_VCO_TUNE_CTRL 0x0108
> +#define QSERDES_COM_VCO_TUNE_MAP 0x010C
> +#define QSERDES_COM_VCO_TUNE1_MODE0 0x0110
> +#define QSERDES_COM_VCO_TUNE2_MODE0 0x0114
> +#define QSERDES_COM_CMN_STATUS 0x0140
> +
> +#define QSERDES_COM_CLK_SEL 0x0154
> +#define QSERDES_COM_HSCLK_SEL 0x0158
> +
> +#define QSERDES_COM_CORECLK_DIV_MODE0 0x0168
> +
> +#define QSERDES_COM_CORE_CLK_EN 0x0174
> +#define QSERDES_COM_C_READY_STATUS 0x0178
> +#define QSERDES_COM_CMN_CONFIG 0x017C
> +
> +#define QSERDES_COM_SVS_MODE_CLK_SEL 0x0184
> +
> +#define QSERDES_COM_SSC_EN_CENTER 0x0010
> +#define QSERDES_COM_SSC_ADJ_PER1 0x0014
> +#define QSERDES_COM_SSC_ADJ_PER2 0x0018
> +#define QSERDES_COM_SSC_PER1 0x001C
> +#define QSERDES_COM_SSC_PER2 0x0020
> +#define QSERDES_COM_SSC_STEP_SIZE1_MODE0 0x0024
> +#define QSERDES_COM_SSC_STEP_SIZE2_MODE0 0x0028
> +
> +#define DP_PHY_PLL_POLL_SLEEP_US 500
> +#define DP_PHY_PLL_POLL_TIMEOUT_US 10000
> +
> +#define EDP_VCO_RATE_8100MHZDIV1000 8100000UL
> +#define EDP_VCO_RATE_8640MHZDIV1000 8640000UL
> +#define EDP_VCO_RATE_9720MHZDIV1000 9720000UL
> +#define EDP_VCO_RATE_10800MHZDIV1000 10800000UL
> +#define EDP_VCO_RATE_11880MHZDIV1000 11880000UL
Can we use phy-qcom-qmp.h for register difinitions?
> +
> +struct qcom_edp {
> + struct device *dev;
> +
> + struct phy *phy;
> +
> + void __iomem *edp;
> + void __iomem *tx0;
> + void __iomem *tx1;
> + void __iomem *pll;
> +
> + struct clk_hw dp_link_hw;
> + struct clk_hw dp_pixel_hw;
> +
> + struct phy_configure_opts_dp dp_opts;
> +
> + struct clk_bulk_data clks[2];
> + struct regulator_bulk_data supplies[2];
> +} ;
> +
> +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_COM_BIAS_EN_CLKBUFLR_EN);
> +
> + 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);
> +
> + writel(0x1f, edp->edp + 0x58);
> +
> + msleep(20);
> +
> + return 0;
> +
> +out_disable_supplies:
> + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies);
> +
> + return ret;
> +}
> +
> +static int qcom_edp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
> +{
> + const struct phy_configure_opts_dp *dp_opts = &opts->dp;
> + struct qcom_edp *edp = phy_get_drvdata(phy);
> +
> + memcpy(&edp->dp_opts, dp_opts, sizeof(*dp_opts));
> +
> + return 0;
> +}
> +
> +static int qcom_edp_configure_ssc(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;
> + case 5400:
> + step1 = 0x5c;
> + step2 = 0x08;
> + break;
> + default:
> + /* Other link rates aren't supported */
> + return -EINVAL;
> + }
> +
> + writel(0x01, edp->pll + QSERDES_COM_SSC_EN_CENTER);
> + writel(0x00, edp->pll + QSERDES_COM_SSC_ADJ_PER1);
> + writel(0x36, edp->pll + QSERDES_COM_SSC_PER1);
> + writel(0x01, edp->pll + QSERDES_COM_SSC_PER2);
> + writel(step1, edp->pll + QSERDES_COM_SSC_STEP_SIZE1_MODE0);
> + writel(step2, edp->pll + QSERDES_COM_SSC_STEP_SIZE2_MODE0);
> +
> + return 0;
> +}
> +
> +static int qcom_edp_configure_pll(struct qcom_edp *edp)
> +{
> + const struct phy_configure_opts_dp *dp_opts = &edp->dp_opts;
> + u32 div_frac_start2_mode0;
> + u32 div_frac_start3_mode0;
> + u32 dec_start_mode0;
> + u32 lock_cmp1_mode0;
> + u32 lock_cmp2_mode0;
> + u32 hsclk_sel;
> +
> + switch (dp_opts->link_rate) {
> + case 1620:
> + hsclk_sel = 0x5;
> + dec_start_mode0 = 0x69;
> + div_frac_start2_mode0 = 0x80;
> + div_frac_start3_mode0 = 0x07;
> + lock_cmp1_mode0 = 0x6f;
> + lock_cmp2_mode0 = 0x08;
> + break;
> + case 2700:
> + hsclk_sel = 0x3;
> + dec_start_mode0 = 0x69;
> + div_frac_start2_mode0 = 0x80;
> + div_frac_start3_mode0 = 0x07;
> + lock_cmp1_mode0 = 0x0f;
> + lock_cmp2_mode0 = 0x0e;
> + break;
> + case 5400:
> + hsclk_sel = 0x1;
> + dec_start_mode0 = 0x8c;
> + div_frac_start2_mode0 = 0x00;
> + div_frac_start3_mode0 = 0x0a;
> + lock_cmp1_mode0 = 0x1f;
> + lock_cmp2_mode0 = 0x1c;
> + break;
> + case 8100:
> + hsclk_sel = 0x0;
> + dec_start_mode0 = 0x69;
> + div_frac_start2_mode0 = 0x80;
> + div_frac_start3_mode0 = 0x07;
> + lock_cmp1_mode0 = 0x2f;
> + lock_cmp2_mode0 = 0x2a;
> + break;
> + default:
> + /* Other link rates aren't supported */
> + return -EINVAL;
> + }
> +
> + writel(0x01, edp->pll + QSERDES_COM_SVS_MODE_CLK_SEL);
> + writel(0x0b, edp->pll + QSERDES_COM_SYSCLK_EN_SEL);
> + writel(0x02, edp->pll + QSERDES_COM_SYS_CLK_CTRL);
> + writel(0x0c, edp->pll + QSERDES_COM_CLK_ENABLE1);
> + writel(0x06, edp->pll + QSERDES_COM_SYSCLK_BUF_ENABLE);
> + writel(0x30, edp->pll + QSERDES_COM_CLK_SEL);
> + writel(hsclk_sel, edp->pll + QSERDES_COM_HSCLK_SEL);
> + writel(0x0f, edp->pll + QSERDES_COM_PLL_IVCO);
> + writel(0x08, edp->pll + QSERDES_COM_LOCK_CMP_EN);
> + writel(0x36, edp->pll + QSERDES_COM_PLL_CCTRL_MODE0);
> + writel(0x16, edp->pll + QSERDES_COM_PLL_RCTRL_MODE0);
> + writel(0x06, edp->pll + QSERDES_COM_CP_CTRL_MODE0);
> + writel(dec_start_mode0, edp->pll + QSERDES_COM_DEC_START_MODE0);
> + writel(0x00, edp->pll + QSERDES_COM_DIV_FRAC_START1_MODE0);
> + writel(div_frac_start2_mode0, edp->pll + QSERDES_COM_DIV_FRAC_START2_MODE0);
> + writel(div_frac_start3_mode0, edp->pll + QSERDES_COM_DIV_FRAC_START3_MODE0);
> + writel(0x02, edp->pll + QSERDES_COM_CMN_CONFIG);
> + writel(0x3f, edp->pll + QSERDES_COM_INTEGLOOP_GAIN0_MODE0);
> + writel(0x00, edp->pll + QSERDES_COM_INTEGLOOP_GAIN1_MODE0);
> + writel(0x00, edp->pll + QSERDES_COM_VCO_TUNE_MAP);
> + writel(lock_cmp1_mode0, edp->pll + QSERDES_COM_LOCK_CMP1_MODE0);
> + writel(lock_cmp2_mode0, edp->pll + QSERDES_COM_LOCK_CMP2_MODE0);
> +
> + writel(0x0a, edp->pll + QSERDES_COM_BG_TIMER);
> + writel(0x14, edp->pll + QSERDES_COM_CORECLK_DIV_MODE0);
> + writel(0x00, edp->pll + QSERDES_COM_VCO_TUNE_CTRL);
> + writel(0x17, edp->pll + QSERDES_COM_BIAS_EN_CLKBUFLR_EN);
> + writel(0x0f, edp->pll + QSERDES_COM_CORE_CLK_EN);
> + writel(0xa0, edp->pll + QSERDES_COM_VCO_TUNE1_MODE0);
> + writel(0x03, edp->pll + QSERDES_COM_VCO_TUNE2_MODE0);
> +
> + return 0;
> +}
> +
> +static int qcom_edp_set_vco_div(struct qcom_edp *edp)
> +{
> + const struct phy_configure_opts_dp *dp_opts = &edp->dp_opts;
> + unsigned long pixel_freq;
> + u32 vco_div;
> +
> + switch (dp_opts->link_rate) {
> + case 1620:
> + vco_div = 0x1;
> + pixel_freq = 1620000000UL / 2;
> + break;
> + case 2700:
> + vco_div = 0x1;
> + pixel_freq = 2700000000UL / 2;
> + break;
> + case 5400:
> + vco_div = 0x2;
> + pixel_freq = 5400000000UL / 4;
> + break;
> + case 8100:
> + vco_div = 0x0;
> + pixel_freq = 8100000000UL / 6;
> + break;
> + default:
> + /* Other link rates aren't supported */
> + return -EINVAL;
> + }
> +
> + writel(vco_div, edp->edp + DP_PHY_VCO_DIV);
> +
> + clk_set_rate(edp->dp_link_hw.clk, dp_opts->link_rate * 100000);
> + clk_set_rate(edp->dp_pixel_hw.clk, pixel_freq);
> +
> + return 0;
> +}
> +
> +static int qcom_edp_phy_power_on(struct phy *phy)
> +{
> + struct qcom_edp *edp = phy_get_drvdata(phy);
> + int timeout;
> + int ret;
> + u32 val;
> +
> + 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(0xfc, edp->edp + DP_PHY_MODE);
> +
> + timeout = readl_poll_timeout(edp->pll + QSERDES_COM_CMN_STATUS,
> + val, val & BIT(7), 5, 200);
> + if (timeout)
> + return timeout;
> +
> + writel(0x01, edp->tx0 + TXn_LDO_CONFIG);
> + writel(0x01, edp->tx1 + TXn_LDO_CONFIG);
> + writel(0x00, edp->tx0 + TXn_LANE_MODE_1);
> + writel(0x00, edp->tx1 + TXn_LANE_MODE_1);
> +
> + ret = qcom_edp_configure_ssc(edp);
> + if (ret)
> + return ret;
> +
> + ret = qcom_edp_configure_pll(edp);
> + if (ret)
> + return ret;
> +
> + /* TX Lane configuration */
> + writel(0x05, edp->edp + DP_PHY_TX0_TX1_LANE_CTL);
> + writel(0x05, edp->edp + DP_PHY_TX2_TX3_LANE_CTL);
> +
> + /* TX-0 register configuration */
> + writel(0x03, edp->tx0 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x0f, edp->tx0 + TXn_CLKBUF_ENABLE);
> + writel(0x03, edp->tx0 + TXn_RESET_TSYNC_EN);
> + writel(0x01, edp->tx0 + TXn_TRAN_DRVR_EMP_EN);
> + writel(0x04, edp->tx0 + TXn_TX_BAND);
> +
> + /* TX-1 register configuration */
> + writel(0x03, edp->tx1 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x0f, edp->tx1 + TXn_CLKBUF_ENABLE);
> + writel(0x03, edp->tx1 + TXn_RESET_TSYNC_EN);
> + writel(0x01, edp->tx1 + TXn_TRAN_DRVR_EMP_EN);
> + writel(0x04, edp->tx1 + TXn_TX_BAND);
> +
> + ret = qcom_edp_set_vco_div(edp);
> + if (ret)
> + return ret;
> +
> + writel(0x01, edp->edp + DP_PHY_CFG);
> + writel(0x05, edp->edp + DP_PHY_CFG);
> + writel(0x01, edp->edp + DP_PHY_CFG);
> + writel(0x09, edp->edp + DP_PHY_CFG);
> +
> + writel(0x20, edp->pll + QSERDES_COM_RESETSM_CNTRL);
> +
> + timeout = readl_poll_timeout(edp->pll + QSERDES_COM_C_READY_STATUS,
> + val, val & BIT(0), 500, 10000);
> + if (timeout)
> + return timeout;
> +
> + writel(0x19, edp->edp + DP_PHY_CFG);
> + writel(0x1f, edp->tx0 + TXn_HIGHZ_DRVR_EN);
> + writel(0x04, edp->tx0 + TXn_HIGHZ_DRVR_EN);
> + writel(0x00, edp->tx0 + TXn_TX_POL_INV);
> + writel(0x1f, edp->tx1 + TXn_HIGHZ_DRVR_EN);
> + writel(0x04, edp->tx1 + TXn_HIGHZ_DRVR_EN);
> + writel(0x00, edp->tx1 + TXn_TX_POL_INV);
> + writel(0x10, edp->tx0 + TXn_TX_DRV_LVL_OFFSET);
> + writel(0x10, edp->tx1 + TXn_TX_DRV_LVL_OFFSET);
> + writel(0x11, edp->tx0 + TXn_RES_CODE_LANE_OFFSET_TX0);
> + writel(0x11, edp->tx0 + TXn_RES_CODE_LANE_OFFSET_TX1);
> + writel(0x11, edp->tx1 + TXn_RES_CODE_LANE_OFFSET_TX0);
> + writel(0x11, edp->tx1 + TXn_RES_CODE_LANE_OFFSET_TX1);
> +
> + writel(0x10, edp->tx0 + TXn_TX_EMP_POST1_LVL);
> + writel(0x10, edp->tx1 + TXn_TX_EMP_POST1_LVL);
> + writel(0x1f, edp->tx0 + TXn_TX_DRV_LVL);
> + writel(0x1f, edp->tx1 + TXn_TX_DRV_LVL);
> +
> + writel(0x4, edp->tx0 + TXn_HIGHZ_DRVR_EN);
> + writel(0x3, edp->tx0 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x4, edp->tx1 + TXn_HIGHZ_DRVR_EN);
> + writel(0x0, edp->tx1 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x3, edp->edp + DP_PHY_CFG_1);
> +
> + writel(0x18, edp->edp + DP_PHY_CFG);
> + udelay(100);
> +
> + writel(0x19, edp->edp + DP_PHY_CFG);
> +
> + return readl_poll_timeout(edp->edp + DP_PHY_STATUS,
> + val, val & BIT(1), 500, 10000);
> +}
> +
> +static int qcom_edp_phy_power_off(struct phy *phy)
> +{
> + struct qcom_edp *edp = phy_get_drvdata(phy);
> +
> + writel(DP_PHY_PD_CTL_PSR_PWRDN, edp->edp + DP_PHY_PD_CTL);
> +
> + return 0;
> +}
> +
> +static int qcom_edp_phy_exit(struct phy *phy)
> +{
> + struct qcom_edp *edp = phy_get_drvdata(phy);
> +
> + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks);
> + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies);
> +
> + return 0;
> +}
> +
> +static const struct phy_ops qcom_edp_ops = {
> + .init = qcom_edp_phy_init,
> + .configure = qcom_edp_phy_configure,
> + .power_on = qcom_edp_phy_power_on,
> + .power_off = qcom_edp_phy_power_off,
> + .exit = qcom_edp_phy_exit,
> + .owner = THIS_MODULE,
> +};
> +
> +/*
> + * Display Port PLL driver block diagram for branch clocks
> + *
> + * +------------------------------+
> + * | DP_VCO_CLK |
> + * | |
> + * | +-------------------+ |
> + * | | (DP PLL/VCO) | |
> + * | +---------+---------+ |
> + * | v |
> + * | +----------+-----------+ |
> + * | | hsclk_divsel_clk_src | |
> + * | +----------+-----------+ |
> + * +------------------------------+
> + * |
> + * +---------<---------v------------>----------+
> + * | |
> + * +--------v----------------+ |
> + * | dp_phy_pll_link_clk | |
> + * | link_clk | |
> + * +--------+----------------+ |
> + * | |
> + * | |
> + * v v
> + * Input to DISPCC block |
> + * for link clk, crypto clk |
> + * and interface clock |
> + * |
> + * |
> + * +--------<------------+-----------------+---<---+
> + * | | |
> + * +----v---------+ +--------v-----+ +--------v------+
> + * | vco_divided | | vco_divided | | vco_divided |
> + * | _clk_src | | _clk_src | | _clk_src |
> + * | | | | | |
> + * |divsel_six | | divsel_two | | divsel_four |
> + * +-------+------+ +-----+--------+ +--------+------+
> + * | | |
> + * v---->----------v-------------<------v
> + * |
> + * +----------+-----------------+
> + * | dp_phy_pll_vco_div_clk |
> + * +---------+------------------+
> + * |
> + * v
> + * Input to DISPCC block
> + * for DP pixel clock
> + *
> + */
> +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 */
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static unsigned long
> +qcom_edp_dp_pixel_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> +{
> + const struct qcom_edp *edp = container_of(hw, struct qcom_edp, dp_pixel_hw);
> + const struct phy_configure_opts_dp *dp_opts = &edp->dp_opts;
> +
> + switch (dp_opts->link_rate) {
> + case 1620:
> + return 1620000000UL / 2;
> + case 2700:
> + return 2700000000UL / 2;
> + case 5400:
> + return 5400000000UL / 4;
> + case 8100:
> + return 8100000000UL / 6;
> + default:
> + return 0;
> + }
> +}
> +
> +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)
> +{
> + switch (req->rate) {
> + case 162000000:
> + case 270000000:
> + case 540000000:
> + case 810000000:
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static unsigned long
> +qcom_edp_dp_link_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> +{
> + const struct qcom_edp *edp = container_of(hw, struct qcom_edp, dp_link_hw);
> + const struct phy_configure_opts_dp *dp_opts = &edp->dp_opts;
> +
> + switch (dp_opts->link_rate) {
> + case 1620:
> + case 2700:
> + case 5400:
> + case 8100:
> + return dp_opts->link_rate * 100000;
> + default:
> + return 0;
> + }
> +}
> +
> +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,
> +};
> +
> +static struct clk_hw *
> +qcom_edp_dp_clks_hw_get(struct of_phandle_args *clkspec, void *data)
> +{
> + unsigned int idx = clkspec->args[0];
> + struct qcom_edp *edp = data;
> +
> + if (idx >= 2) {
> + pr_err("%s: invalid index %u\n", __func__, idx);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (idx == 0)
> + return &edp->dp_link_hw;
> +
> + return &edp->dp_pixel_hw;
> +}
> +
> +static void phy_clk_release_provider(void *res)
> +{
> + of_clk_del_provider(res);
> +}
> +
> +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);
I think we can use of_clk_onecell_get here (and we should change main
qmp driver)
Also we can use devm_of_clk_add_hw_provider.
> + if (ret)
> + return ret;
> +
> + /*
> + * Roll a devm action because the clock provider is the child node, but
> + * 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;
> +}
> +
> +static int qcom_edp_phy_probe(struct platform_device *pdev)
> +{
> + struct phy_provider *phy_provider;
> + struct device *dev = &pdev->dev;
> + struct qcom_edp *edp;
> + int ret;
> +
> + edp = devm_kzalloc(dev, sizeof(*edp), GFP_KERNEL);
> + if (!edp)
> + return -ENOMEM;
> +
> + edp->dev = dev;
> +
> + edp->edp = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(edp->edp))
> + return PTR_ERR(edp->edp);
> +
> + edp->tx0 = devm_platform_ioremap_resource(pdev, 1);
> + if (IS_ERR(edp->tx0))
> + return PTR_ERR(edp->tx0);
> +
> + edp->tx1 = devm_platform_ioremap_resource(pdev, 2);
> + if (IS_ERR(edp->tx1))
> + return PTR_ERR(edp->tx1);
> +
> + edp->pll = devm_platform_ioremap_resource(pdev, 3);
> + if (IS_ERR(edp->pll))
> + return PTR_ERR(edp->pll);
> +
> + edp->clks[0].id = "aux";
> + edp->clks[1].id = "cfg_ahb";
> + ret = devm_clk_bulk_get(dev, ARRAY_SIZE(edp->clks), edp->clks);
> + if (ret)
> + return ret;
> +
> + edp->supplies[0].supply = "vdda-phy";
> + edp->supplies[1].supply = "vdda-pll";
> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(edp->supplies), edp->supplies);
> + if (ret)
> + return ret;
> +
> + ret = qcom_edp_clks_register(edp, pdev->dev.of_node);
> + if (ret)
> + return ret;
> +
> + edp->phy = devm_phy_create(dev, pdev->dev.of_node, &qcom_edp_ops);
> + if (IS_ERR(edp->phy)) {
> + dev_err(dev, "failed to register phy\n");
> + return PTR_ERR(edp->phy);
> + }
> +
> + phy_set_drvdata(edp->phy, edp);
> +
> + phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> + return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static const struct of_device_id qcom_edp_phy_match_table[] = {
> + { .compatible = "qcom,sc8180x-edp-phy" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table);
> +
> +static struct platform_driver qcom_edp_phy_driver = {
> + .probe = qcom_edp_phy_probe,
> + .driver = {
> + .name = "qcom-edp-phy",
> + .of_match_table = qcom_edp_phy_match_table,
> + },
> +};
> +
> +module_platform_driver(qcom_edp_phy_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm eDP PHY driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.29.2
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver
2021-05-11 4:19 ` [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver Bjorn Andersson
` (2 preceding siblings ...)
2021-05-11 14:46 ` Dmitry Baryshkov
@ 2021-05-14 10:40 ` Vinod Koul
2021-08-10 3:15 ` sbillaka
4 siblings, 0 replies; 12+ messages in thread
From: Vinod Koul @ 2021-05-14 10:40 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Kishon Vijay Abraham I, Rob Herring, sbillaka,
Rob Clark, Dmitry Baryshkov, Stephen Boyd, linux-arm-msm,
linux-phy, devicetree, linux-kernel
On 10-05-21, 23:19, Bjorn Andersson wrote:
> +static int qcom_edp_phy_power_on(struct phy *phy)
> +{
> + struct qcom_edp *edp = phy_get_drvdata(phy);
> + int timeout;
> + int ret;
> + u32 val;
> +
> + 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(0xfc, edp->edp + DP_PHY_MODE);
> +
> + timeout = readl_poll_timeout(edp->pll + QSERDES_COM_CMN_STATUS,
> + val, val & BIT(7), 5, 200);
> + if (timeout)
> + return timeout;
> +
> + writel(0x01, edp->tx0 + TXn_LDO_CONFIG);
> + writel(0x01, edp->tx1 + TXn_LDO_CONFIG);
> + writel(0x00, edp->tx0 + TXn_LANE_MODE_1);
> + writel(0x00, edp->tx1 + TXn_LANE_MODE_1);
> +
> + ret = qcom_edp_configure_ssc(edp);
> + if (ret)
> + return ret;
> +
> + ret = qcom_edp_configure_pll(edp);
> + if (ret)
> + return ret;
> +
> + /* TX Lane configuration */
> + writel(0x05, edp->edp + DP_PHY_TX0_TX1_LANE_CTL);
> + writel(0x05, edp->edp + DP_PHY_TX2_TX3_LANE_CTL);
> +
> + /* TX-0 register configuration */
> + writel(0x03, edp->tx0 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x0f, edp->tx0 + TXn_CLKBUF_ENABLE);
> + writel(0x03, edp->tx0 + TXn_RESET_TSYNC_EN);
> + writel(0x01, edp->tx0 + TXn_TRAN_DRVR_EMP_EN);
> + writel(0x04, edp->tx0 + TXn_TX_BAND);
> +
> + /* TX-1 register configuration */
> + writel(0x03, edp->tx1 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x0f, edp->tx1 + TXn_CLKBUF_ENABLE);
> + writel(0x03, edp->tx1 + TXn_RESET_TSYNC_EN);
> + writel(0x01, edp->tx1 + TXn_TRAN_DRVR_EMP_EN);
> + writel(0x04, edp->tx1 + TXn_TX_BAND);
> +
> + ret = qcom_edp_set_vco_div(edp);
> + if (ret)
> + return ret;
> +
> + writel(0x01, edp->edp + DP_PHY_CFG);
> + writel(0x05, edp->edp + DP_PHY_CFG);
> + writel(0x01, edp->edp + DP_PHY_CFG);
> + writel(0x09, edp->edp + DP_PHY_CFG);
> +
> + writel(0x20, edp->pll + QSERDES_COM_RESETSM_CNTRL);
> +
> + timeout = readl_poll_timeout(edp->pll + QSERDES_COM_C_READY_STATUS,
> + val, val & BIT(0), 500, 10000);
> + if (timeout)
> + return timeout;
> +
> + writel(0x19, edp->edp + DP_PHY_CFG);
> + writel(0x1f, edp->tx0 + TXn_HIGHZ_DRVR_EN);
> + writel(0x04, edp->tx0 + TXn_HIGHZ_DRVR_EN);
> + writel(0x00, edp->tx0 + TXn_TX_POL_INV);
> + writel(0x1f, edp->tx1 + TXn_HIGHZ_DRVR_EN);
> + writel(0x04, edp->tx1 + TXn_HIGHZ_DRVR_EN);
> + writel(0x00, edp->tx1 + TXn_TX_POL_INV);
> + writel(0x10, edp->tx0 + TXn_TX_DRV_LVL_OFFSET);
> + writel(0x10, edp->tx1 + TXn_TX_DRV_LVL_OFFSET);
> + writel(0x11, edp->tx0 + TXn_RES_CODE_LANE_OFFSET_TX0);
> + writel(0x11, edp->tx0 + TXn_RES_CODE_LANE_OFFSET_TX1);
> + writel(0x11, edp->tx1 + TXn_RES_CODE_LANE_OFFSET_TX0);
> + writel(0x11, edp->tx1 + TXn_RES_CODE_LANE_OFFSET_TX1);
> +
> + writel(0x10, edp->tx0 + TXn_TX_EMP_POST1_LVL);
> + writel(0x10, edp->tx1 + TXn_TX_EMP_POST1_LVL);
> + writel(0x1f, edp->tx0 + TXn_TX_DRV_LVL);
> + writel(0x1f, edp->tx1 + TXn_TX_DRV_LVL);
> +
> + writel(0x4, edp->tx0 + TXn_HIGHZ_DRVR_EN);
> + writel(0x3, edp->tx0 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x4, edp->tx1 + TXn_HIGHZ_DRVR_EN);
> + writel(0x0, edp->tx1 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x3, edp->edp + DP_PHY_CFG_1);
> +
> + writel(0x18, edp->edp + DP_PHY_CFG);
Can't help but think that this should be made modular. We would come up
with next set which would have different set of sequence/values so
having a qmp style table here would be better..
--
~Vinod
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver
2021-05-11 4:19 ` [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver Bjorn Andersson
` (3 preceding siblings ...)
2021-05-14 10:40 ` Vinod Koul
@ 2021-08-10 3:15 ` sbillaka
2021-08-10 3:36 ` Bjorn Andersson
4 siblings, 1 reply; 12+ messages in thread
From: sbillaka @ 2021-08-10 3:15 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Kishon Vijay Abraham I, Vinod Koul, Rob Herring,
Rob Clark, Dmitry Baryshkov, Stephen Boyd, linux-arm-msm,
linux-phy, devicetree, linux-kernel, kalyan_t, abhinavk
Hello Bjorn,
On 2021-05-11 09:49, Bjorn Andersson wrote:
> 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.
>
> 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.
>
> 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
> + tristate "Qualcomm eDP PHY driver"
> + depends on ARCH_QCOM || COMPILE_TEST
> + 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
> 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
The DP_PHY_AUX_CFG0 offset for sc8180x eDP phy is 0x0024.
Some of the eDP PHY offset addresses are shifted by 4 address locations,
compared to the DP QMP PHY offset addresses for sc8180x.
The DP_PHY_AUX_CFG* offsets for this eDP phy driver are as below:
#define DP_PHY_AUX_CFG0 0x0024
#define DP_PHY_AUX_CFG1 0x0028
#define DP_PHY_AUX_CFG2 0x002c
#define DP_PHY_AUX_CFG3 0x0030
#define DP_PHY_AUX_CFG4 0x0034
#define DP_PHY_AUX_CFG5 0x0038
#define DP_PHY_AUX_CFG6 0x003c
#define DP_PHY_AUX_CFG7 0x0040
#define DP_PHY_AUX_CFG8 0x0044
#define DP_PHY_AUX_CFG9 0x0048
> +
> +#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
> +
> +#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
> +#define QSERDES_COM_SYSCLK_BUF_ENABLE 0x0050
> +#define QSERDES_COM_PLL_IVCO 0x0058
> +
> +#define QSERDES_COM_CP_CTRL_MODE0 0x0074
> +#define QSERDES_COM_PLL_RCTRL_MODE0 0x007C
> +#define QSERDES_COM_PLL_CCTRL_MODE0 0x0084
> +#define QSERDES_COM_SYSCLK_EN_SEL 0x0094
> +#define QSERDES_COM_RESETSM_CNTRL 0x009C
> +#define QSERDES_COM_LOCK_CMP_EN 0x00A4
> +#define QSERDES_COM_LOCK_CMP1_MODE0 0x00AC
> +#define QSERDES_COM_LOCK_CMP2_MODE0 0x00B0
> +
> +#define QSERDES_COM_DEC_START_MODE0 0x00BC
> +#define QSERDES_COM_DIV_FRAC_START1_MODE0 0x00CC
> +#define QSERDES_COM_DIV_FRAC_START2_MODE0 0x00D0
> +#define QSERDES_COM_DIV_FRAC_START3_MODE0 0x00D4
> +#define QSERDES_COM_INTEGLOOP_GAIN0_MODE0 0x00EC
> +#define QSERDES_COM_INTEGLOOP_GAIN1_MODE0 0x00F0
> +#define QSERDES_COM_VCO_TUNE_CTRL 0x0108
> +#define QSERDES_COM_VCO_TUNE_MAP 0x010C
> +#define QSERDES_COM_VCO_TUNE1_MODE0 0x0110
> +#define QSERDES_COM_VCO_TUNE2_MODE0 0x0114
> +#define QSERDES_COM_CMN_STATUS 0x0140
> +
> +#define QSERDES_COM_CLK_SEL 0x0154
> +#define QSERDES_COM_HSCLK_SEL 0x0158
> +
> +#define QSERDES_COM_CORECLK_DIV_MODE0 0x0168
> +
> +#define QSERDES_COM_CORE_CLK_EN 0x0174
> +#define QSERDES_COM_C_READY_STATUS 0x0178
> +#define QSERDES_COM_CMN_CONFIG 0x017C
> +
> +#define QSERDES_COM_SVS_MODE_CLK_SEL 0x0184
> +
> +#define QSERDES_COM_SSC_EN_CENTER 0x0010
> +#define QSERDES_COM_SSC_ADJ_PER1 0x0014
> +#define QSERDES_COM_SSC_ADJ_PER2 0x0018
> +#define QSERDES_COM_SSC_PER1 0x001C
> +#define QSERDES_COM_SSC_PER2 0x0020
> +#define QSERDES_COM_SSC_STEP_SIZE1_MODE0 0x0024
> +#define QSERDES_COM_SSC_STEP_SIZE2_MODE0 0x0028
> +
> +#define DP_PHY_PLL_POLL_SLEEP_US 500
> +#define DP_PHY_PLL_POLL_TIMEOUT_US 10000
> +
> +#define EDP_VCO_RATE_8100MHZDIV1000 8100000UL
> +#define EDP_VCO_RATE_8640MHZDIV1000 8640000UL
> +#define EDP_VCO_RATE_9720MHZDIV1000 9720000UL
> +#define EDP_VCO_RATE_10800MHZDIV1000 10800000UL
> +#define EDP_VCO_RATE_11880MHZDIV1000 11880000UL
> +
> +struct qcom_edp {
> + struct device *dev;
> +
> + struct phy *phy;
> +
> + void __iomem *edp;
> + void __iomem *tx0;
> + void __iomem *tx1;
> + void __iomem *pll;
> +
> + struct clk_hw dp_link_hw;
> + struct clk_hw dp_pixel_hw;
> +
> + struct phy_configure_opts_dp dp_opts;
> +
> + struct clk_bulk_data clks[2];
> + struct regulator_bulk_data supplies[2];
> +} ;
> +
> +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;
I think the number of clk and regulator resources can vary based on
platform.
> +
> + 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_COM_BIAS_EN_CLKBUFLR_EN);
> +
> + 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);
> +
> + writel(0x1f, edp->edp + 0x58);
> +
> + msleep(20);
> +
> + return 0;
> +
> +out_disable_supplies:
> + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies);
> +
> + return ret;
> +}
> +
> +static int qcom_edp_phy_configure(struct phy *phy, union
> phy_configure_opts *opts)
> +{
> + const struct phy_configure_opts_dp *dp_opts = &opts->dp;
> + struct qcom_edp *edp = phy_get_drvdata(phy);
> +
> + memcpy(&edp->dp_opts, dp_opts, sizeof(*dp_opts));
> +
> + return 0;
> +}
> +
> +static int qcom_edp_configure_ssc(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;
> + case 5400:
> + step1 = 0x5c;
> + step2 = 0x08;
> + break;
> + default:
> + /* Other link rates aren't supported */
> + return -EINVAL;
> + }
> +
> + writel(0x01, edp->pll + QSERDES_COM_SSC_EN_CENTER);
> + writel(0x00, edp->pll + QSERDES_COM_SSC_ADJ_PER1);
> + writel(0x36, edp->pll + QSERDES_COM_SSC_PER1);
> + writel(0x01, edp->pll + QSERDES_COM_SSC_PER2);
> + writel(step1, edp->pll + QSERDES_COM_SSC_STEP_SIZE1_MODE0);
> + writel(step2, edp->pll + QSERDES_COM_SSC_STEP_SIZE2_MODE0);
> +
> + return 0;
> +}
> +
> +static int qcom_edp_configure_pll(struct qcom_edp *edp)
> +{
> + const struct phy_configure_opts_dp *dp_opts = &edp->dp_opts;
> + u32 div_frac_start2_mode0;
> + u32 div_frac_start3_mode0;
> + u32 dec_start_mode0;
> + u32 lock_cmp1_mode0;
> + u32 lock_cmp2_mode0;
> + u32 hsclk_sel;
> +
> + switch (dp_opts->link_rate) {
> + case 1620:
> + hsclk_sel = 0x5;
> + dec_start_mode0 = 0x69;
> + div_frac_start2_mode0 = 0x80;
> + div_frac_start3_mode0 = 0x07;
> + lock_cmp1_mode0 = 0x6f;
> + lock_cmp2_mode0 = 0x08;
> + break;
> + case 2700:
> + hsclk_sel = 0x3;
> + dec_start_mode0 = 0x69;
> + div_frac_start2_mode0 = 0x80;
> + div_frac_start3_mode0 = 0x07;
> + lock_cmp1_mode0 = 0x0f;
> + lock_cmp2_mode0 = 0x0e;
> + break;
> + case 5400:
> + hsclk_sel = 0x1;
> + dec_start_mode0 = 0x8c;
> + div_frac_start2_mode0 = 0x00;
> + div_frac_start3_mode0 = 0x0a;
> + lock_cmp1_mode0 = 0x1f;
> + lock_cmp2_mode0 = 0x1c;
> + break;
> + case 8100:
> + hsclk_sel = 0x0;
> + dec_start_mode0 = 0x69;
> + div_frac_start2_mode0 = 0x80;
> + div_frac_start3_mode0 = 0x07;
> + lock_cmp1_mode0 = 0x2f;
> + lock_cmp2_mode0 = 0x2a;
> + break;
> + default:
> + /* Other link rates aren't supported */
> + return -EINVAL;
> + }
> +
> + writel(0x01, edp->pll + QSERDES_COM_SVS_MODE_CLK_SEL);
> + writel(0x0b, edp->pll + QSERDES_COM_SYSCLK_EN_SEL);
> + writel(0x02, edp->pll + QSERDES_COM_SYS_CLK_CTRL);
> + writel(0x0c, edp->pll + QSERDES_COM_CLK_ENABLE1);
> + writel(0x06, edp->pll + QSERDES_COM_SYSCLK_BUF_ENABLE);
> + writel(0x30, edp->pll + QSERDES_COM_CLK_SEL);
> + writel(hsclk_sel, edp->pll + QSERDES_COM_HSCLK_SEL);
> + writel(0x0f, edp->pll + QSERDES_COM_PLL_IVCO);
> + writel(0x08, edp->pll + QSERDES_COM_LOCK_CMP_EN);
> + writel(0x36, edp->pll + QSERDES_COM_PLL_CCTRL_MODE0);
> + writel(0x16, edp->pll + QSERDES_COM_PLL_RCTRL_MODE0);
> + writel(0x06, edp->pll + QSERDES_COM_CP_CTRL_MODE0);
> + writel(dec_start_mode0, edp->pll + QSERDES_COM_DEC_START_MODE0);
> + writel(0x00, edp->pll + QSERDES_COM_DIV_FRAC_START1_MODE0);
> + writel(div_frac_start2_mode0, edp->pll +
> QSERDES_COM_DIV_FRAC_START2_MODE0);
> + writel(div_frac_start3_mode0, edp->pll +
> QSERDES_COM_DIV_FRAC_START3_MODE0);
> + writel(0x02, edp->pll + QSERDES_COM_CMN_CONFIG);
> + writel(0x3f, edp->pll + QSERDES_COM_INTEGLOOP_GAIN0_MODE0);
> + writel(0x00, edp->pll + QSERDES_COM_INTEGLOOP_GAIN1_MODE0);
> + writel(0x00, edp->pll + QSERDES_COM_VCO_TUNE_MAP);
> + writel(lock_cmp1_mode0, edp->pll + QSERDES_COM_LOCK_CMP1_MODE0);
> + writel(lock_cmp2_mode0, edp->pll + QSERDES_COM_LOCK_CMP2_MODE0);
> +
> + writel(0x0a, edp->pll + QSERDES_COM_BG_TIMER);
> + writel(0x14, edp->pll + QSERDES_COM_CORECLK_DIV_MODE0);
> + writel(0x00, edp->pll + QSERDES_COM_VCO_TUNE_CTRL);
> + writel(0x17, edp->pll + QSERDES_COM_BIAS_EN_CLKBUFLR_EN);
> + writel(0x0f, edp->pll + QSERDES_COM_CORE_CLK_EN);
> + writel(0xa0, edp->pll + QSERDES_COM_VCO_TUNE1_MODE0);
> + writel(0x03, edp->pll + QSERDES_COM_VCO_TUNE2_MODE0);
> +
> + return 0;
> +}
> +
> +static int qcom_edp_set_vco_div(struct qcom_edp *edp)
> +{
> + const struct phy_configure_opts_dp *dp_opts = &edp->dp_opts;
> + unsigned long pixel_freq;
> + u32 vco_div;
> +
> + switch (dp_opts->link_rate) {
> + case 1620:
> + vco_div = 0x1;
> + pixel_freq = 1620000000UL / 2;
> + break;
> + case 2700:
> + vco_div = 0x1;
> + pixel_freq = 2700000000UL / 2;
> + break;
> + case 5400:
> + vco_div = 0x2;
> + pixel_freq = 5400000000UL / 4;
> + break;
> + case 8100:
> + vco_div = 0x0;
> + pixel_freq = 8100000000UL / 6;
> + break;
> + default:
> + /* Other link rates aren't supported */
> + return -EINVAL;
> + }
> +
> + writel(vco_div, edp->edp + DP_PHY_VCO_DIV);
> +
> + clk_set_rate(edp->dp_link_hw.clk, dp_opts->link_rate * 100000);
> + clk_set_rate(edp->dp_pixel_hw.clk, pixel_freq);
> +
> + return 0;
> +}
> +
> +static int qcom_edp_phy_power_on(struct phy *phy)
> +{
> + struct qcom_edp *edp = phy_get_drvdata(phy);
> + int timeout;
> + int ret;
> + u32 val;
> +
> + 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(0xfc, edp->edp + DP_PHY_MODE);
> +
> + timeout = readl_poll_timeout(edp->pll + QSERDES_COM_CMN_STATUS,
> + val, val & BIT(7), 5, 200);
> + if (timeout)
> + return timeout;
> +
> + writel(0x01, edp->tx0 + TXn_LDO_CONFIG);
> + writel(0x01, edp->tx1 + TXn_LDO_CONFIG);
> + writel(0x00, edp->tx0 + TXn_LANE_MODE_1);
> + writel(0x00, edp->tx1 + TXn_LANE_MODE_1);
> +
> + ret = qcom_edp_configure_ssc(edp);
> + if (ret)
> + return ret;
> +
> + ret = qcom_edp_configure_pll(edp);
> + if (ret)
> + return ret;
> +
> + /* TX Lane configuration */
> + writel(0x05, edp->edp + DP_PHY_TX0_TX1_LANE_CTL);
> + writel(0x05, edp->edp + DP_PHY_TX2_TX3_LANE_CTL);
> +
> + /* TX-0 register configuration */
> + writel(0x03, edp->tx0 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x0f, edp->tx0 + TXn_CLKBUF_ENABLE);
> + writel(0x03, edp->tx0 + TXn_RESET_TSYNC_EN);
> + writel(0x01, edp->tx0 + TXn_TRAN_DRVR_EMP_EN);
> + writel(0x04, edp->tx0 + TXn_TX_BAND);
> +
> + /* TX-1 register configuration */
> + writel(0x03, edp->tx1 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x0f, edp->tx1 + TXn_CLKBUF_ENABLE);
> + writel(0x03, edp->tx1 + TXn_RESET_TSYNC_EN);
> + writel(0x01, edp->tx1 + TXn_TRAN_DRVR_EMP_EN);
> + writel(0x04, edp->tx1 + TXn_TX_BAND);
> +
> + ret = qcom_edp_set_vco_div(edp);
> + if (ret)
> + return ret;
> +
> + writel(0x01, edp->edp + DP_PHY_CFG);
> + writel(0x05, edp->edp + DP_PHY_CFG);
> + writel(0x01, edp->edp + DP_PHY_CFG);
> + writel(0x09, edp->edp + DP_PHY_CFG);
> +
> + writel(0x20, edp->pll + QSERDES_COM_RESETSM_CNTRL);
> +
> + timeout = readl_poll_timeout(edp->pll + QSERDES_COM_C_READY_STATUS,
> + val, val & BIT(0), 500, 10000);
> + if (timeout)
> + return timeout;
> +
> + writel(0x19, edp->edp + DP_PHY_CFG);
> + writel(0x1f, edp->tx0 + TXn_HIGHZ_DRVR_EN);
> + writel(0x04, edp->tx0 + TXn_HIGHZ_DRVR_EN);
> + writel(0x00, edp->tx0 + TXn_TX_POL_INV);
> + writel(0x1f, edp->tx1 + TXn_HIGHZ_DRVR_EN);
> + writel(0x04, edp->tx1 + TXn_HIGHZ_DRVR_EN);
> + writel(0x00, edp->tx1 + TXn_TX_POL_INV);
> + writel(0x10, edp->tx0 + TXn_TX_DRV_LVL_OFFSET);
> + writel(0x10, edp->tx1 + TXn_TX_DRV_LVL_OFFSET);
> + writel(0x11, edp->tx0 + TXn_RES_CODE_LANE_OFFSET_TX0);
> + writel(0x11, edp->tx0 + TXn_RES_CODE_LANE_OFFSET_TX1);
> + writel(0x11, edp->tx1 + TXn_RES_CODE_LANE_OFFSET_TX0);
> + writel(0x11, edp->tx1 + TXn_RES_CODE_LANE_OFFSET_TX1);
> +
> + writel(0x10, edp->tx0 + TXn_TX_EMP_POST1_LVL);
> + writel(0x10, edp->tx1 + TXn_TX_EMP_POST1_LVL);
> + writel(0x1f, edp->tx0 + TXn_TX_DRV_LVL);
> + writel(0x1f, edp->tx1 + TXn_TX_DRV_LVL);
> +
> + writel(0x4, edp->tx0 + TXn_HIGHZ_DRVR_EN);
> + writel(0x3, edp->tx0 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x4, edp->tx1 + TXn_HIGHZ_DRVR_EN);
> + writel(0x0, edp->tx1 + TXn_TRANSCEIVER_BIAS_EN);
> + writel(0x3, edp->edp + DP_PHY_CFG_1);
> +
> + writel(0x18, edp->edp + DP_PHY_CFG);
> + udelay(100);
> +
> + writel(0x19, edp->edp + DP_PHY_CFG);
> +
> + return readl_poll_timeout(edp->edp + DP_PHY_STATUS,
> + val, val & BIT(1), 500, 10000);
> +}
> +
> +static int qcom_edp_phy_power_off(struct phy *phy)
> +{
> + struct qcom_edp *edp = phy_get_drvdata(phy);
> +
> + writel(DP_PHY_PD_CTL_PSR_PWRDN, edp->edp + DP_PHY_PD_CTL);
> +
> + return 0;
> +}
> +
> +static int qcom_edp_phy_exit(struct phy *phy)
> +{
> + struct qcom_edp *edp = phy_get_drvdata(phy);
> +
> + clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks);
> + regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies);
I think the number of regulator and clk resources can vary based on
platform.
> +
> + return 0;
> +}
> +
> +static const struct phy_ops qcom_edp_ops = {
> + .init = qcom_edp_phy_init,
> + .configure = qcom_edp_phy_configure,
> + .power_on = qcom_edp_phy_power_on,
> + .power_off = qcom_edp_phy_power_off,
> + .exit = qcom_edp_phy_exit,
> + .owner = THIS_MODULE,
> +};
> +
> +/*
> + * Display Port PLL driver block diagram for branch clocks
> + *
> + * +------------------------------+
> + * | DP_VCO_CLK |
> + * | |
> + * | +-------------------+ |
> + * | | (DP PLL/VCO) | |
> + * | +---------+---------+ |
> + * | v |
> + * | +----------+-----------+ |
> + * | | hsclk_divsel_clk_src | |
> + * | +----------+-----------+ |
> + * +------------------------------+
> + * |
> + * +---------<---------v------------>----------+
> + * | |
> + * +--------v----------------+ |
> + * | dp_phy_pll_link_clk | |
> + * | link_clk | |
> + * +--------+----------------+ |
> + * | |
> + * | |
> + * v v
> + * Input to DISPCC block |
> + * for link clk, crypto clk |
> + * and interface clock |
> + * |
> + * |
> + * +--------<------------+-----------------+---<---+
> + * | | |
> + * +----v---------+ +--------v-----+ +--------v------+
> + * | vco_divided | | vco_divided | | vco_divided |
> + * | _clk_src | | _clk_src | | _clk_src |
> + * | | | | | |
> + * |divsel_six | | divsel_two | | divsel_four |
> + * +-------+------+ +-----+--------+ +--------+------+
> + * | | |
> + * v---->----------v-------------<------v
> + * |
> + * +----------+-----------------+
> + * | dp_phy_pll_vco_div_clk |
> + * +---------+------------------+
> + * |
> + * v
> + * Input to DISPCC block
> + * for DP pixel clock
> + *
> + */
> +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
> */
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static unsigned long
> +qcom_edp_dp_pixel_clk_recalc_rate(struct clk_hw *hw, unsigned long
> parent_rate)
> +{
> + const struct qcom_edp *edp = container_of(hw, struct qcom_edp,
> dp_pixel_hw);
> + const struct phy_configure_opts_dp *dp_opts = &edp->dp_opts;
> +
> + switch (dp_opts->link_rate) {
> + case 1620:
> + return 1620000000UL / 2;
> + case 2700:
> + return 2700000000UL / 2;
> + case 5400:
> + return 5400000000UL / 4;
> + case 8100:
> + return 8100000000UL / 6;
> + default:
> + return 0;
> + }
> +}
> +
> +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)
> +{
> + switch (req->rate) {
> + case 162000000:
> + case 270000000:
> + case 540000000:
> + case 810000000:
> + return 0;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static unsigned long
> +qcom_edp_dp_link_clk_recalc_rate(struct clk_hw *hw, unsigned long
> parent_rate)
> +{
> + const struct qcom_edp *edp = container_of(hw, struct qcom_edp,
> dp_link_hw);
> + const struct phy_configure_opts_dp *dp_opts = &edp->dp_opts;
> +
> + switch (dp_opts->link_rate) {
> + case 1620:
> + case 2700:
> + case 5400:
> + case 8100:
> + return dp_opts->link_rate * 100000;
> + default:
> + return 0;
> + }
> +}
> +
> +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,
> +};
> +
> +static struct clk_hw *
> +qcom_edp_dp_clks_hw_get(struct of_phandle_args *clkspec, void *data)
> +{
> + unsigned int idx = clkspec->args[0];
> + struct qcom_edp *edp = data;
> +
> + if (idx >= 2) {
> + pr_err("%s: invalid index %u\n", __func__, idx);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (idx == 0)
> + return &edp->dp_link_hw;
> +
> + return &edp->dp_pixel_hw;
> +}
> +
> +static void phy_clk_release_provider(void *res)
> +{
> + of_clk_del_provider(res);
> +}
> +
> +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
> + * 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;
> +}
> +
> +static int qcom_edp_phy_probe(struct platform_device *pdev)
> +{
> + struct phy_provider *phy_provider;
> + struct device *dev = &pdev->dev;
> + struct qcom_edp *edp;
> + int ret;
> +
> + edp = devm_kzalloc(dev, sizeof(*edp), GFP_KERNEL);
> + if (!edp)
> + return -ENOMEM;
> +
> + edp->dev = dev;
> +
> + edp->edp = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(edp->edp))
> + return PTR_ERR(edp->edp);
> +
> + edp->tx0 = devm_platform_ioremap_resource(pdev, 1);
> + if (IS_ERR(edp->tx0))
> + return PTR_ERR(edp->tx0);
> +
> + edp->tx1 = devm_platform_ioremap_resource(pdev, 2);
> + if (IS_ERR(edp->tx1))
> + return PTR_ERR(edp->tx1);
> +
> + edp->pll = devm_platform_ioremap_resource(pdev, 3);
> + if (IS_ERR(edp->pll))
> + return PTR_ERR(edp->pll);
> +
> + edp->clks[0].id = "aux";
> + edp->clks[1].id = "cfg_ahb";
> + ret = devm_clk_bulk_get(dev, ARRAY_SIZE(edp->clks), edp->clks);
> + if (ret)
> + return ret;
> +
> + edp->supplies[0].supply = "vdda-phy";
> + edp->supplies[1].supply = "vdda-pll";
> + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(edp->supplies),
> edp->supplies);
> + if (ret)
> + return ret;
I believe, the combination of the number of regulator and clk resources
may vary based on the platform.
I think we should not fail probe if all these resources are not present
in the device tree file.
I think, these resources can be optional. We can get these resources if
they are present in the device tree file and enable them as required.
> +
> + ret = qcom_edp_clks_register(edp, pdev->dev.of_node);
> + if (ret)
> + return ret;
> +
> + edp->phy = devm_phy_create(dev, pdev->dev.of_node, &qcom_edp_ops);
> + if (IS_ERR(edp->phy)) {
> + dev_err(dev, "failed to register phy\n");
> + return PTR_ERR(edp->phy);
> + }
> +
> + phy_set_drvdata(edp->phy, edp);
> +
> + phy_provider = devm_of_phy_provider_register(dev,
> of_phy_simple_xlate);
> + return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static const struct of_device_id qcom_edp_phy_match_table[] = {
> + { .compatible = "qcom,sc8180x-edp-phy" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_edp_phy_match_table);
> +
> +static struct platform_driver qcom_edp_phy_driver = {
> + .probe = qcom_edp_phy_probe,
> + .driver = {
> + .name = "qcom-edp-phy",
> + .of_match_table = qcom_edp_phy_match_table,
> + },
> +};
> +
> +module_platform_driver(qcom_edp_phy_driver);
> +
> +MODULE_DESCRIPTION("Qualcomm eDP PHY driver");
> +MODULE_LICENSE("GPL v2");
Thank you,
Sankeerth
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver
2021-08-10 3:15 ` sbillaka
@ 2021-08-10 3:36 ` Bjorn Andersson
2021-08-12 0:23 ` sbillaka
0 siblings, 1 reply; 12+ messages in thread
From: Bjorn Andersson @ 2021-08-10 3:36 UTC (permalink / raw)
To: sbillaka
Cc: Andy Gross, Kishon Vijay Abraham I, Vinod Koul, Rob Herring,
Rob Clark, Dmitry Baryshkov, Stephen Boyd, linux-arm-msm,
linux-phy, devicetree, linux-kernel, kalyan_t, abhinavk
On Mon 09 Aug 22:15 CDT 2021, sbillaka@codeaurora.org wrote:
> On 2021-05-11 09:49, Bjorn Andersson wrote:
> > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c
[..]
> > +#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
>
> The DP_PHY_AUX_CFG0 offset for sc8180x eDP phy is 0x0024.
> Some of the eDP PHY offset addresses are shifted by 4 address locations,
> compared to the DP QMP PHY offset addresses for sc8180x.
> The DP_PHY_AUX_CFG* offsets for this eDP phy driver are as below:
>
> #define DP_PHY_AUX_CFG0 0x0024
> #define DP_PHY_AUX_CFG1 0x0028
> #define DP_PHY_AUX_CFG2 0x002c
> #define DP_PHY_AUX_CFG3 0x0030
> #define DP_PHY_AUX_CFG4 0x0034
> #define DP_PHY_AUX_CFG5 0x0038
> #define DP_PHY_AUX_CFG6 0x003c
> #define DP_PHY_AUX_CFG7 0x0040
> #define DP_PHY_AUX_CFG8 0x0044
> #define DP_PHY_AUX_CFG9 0x0048
>
I noticed this as well. During development I just used the numbers
directly in the code and I must have screwed up as I replaced them with
defined - and somehow missed this in the testing before posting.
Sorry about that.
[..]
> > +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;
>
> I think the number of clk and regulator resources can vary based on
> platform.
>
If that's the case we should replace the ARRAY_SIZE() with an integer.
But I prefer to wait with that until the number actually is variable.
[..]
> > +static int qcom_edp_phy_probe(struct platform_device *pdev)
> > +{
> > + struct phy_provider *phy_provider;
> > + struct device *dev = &pdev->dev;
> > + struct qcom_edp *edp;
> > + int ret;
> > +
> > + edp = devm_kzalloc(dev, sizeof(*edp), GFP_KERNEL);
> > + if (!edp)
> > + return -ENOMEM;
> > +
> > + edp->dev = dev;
> > +
> > + edp->edp = devm_platform_ioremap_resource(pdev, 0);
> > + if (IS_ERR(edp->edp))
> > + return PTR_ERR(edp->edp);
> > +
> > + edp->tx0 = devm_platform_ioremap_resource(pdev, 1);
> > + if (IS_ERR(edp->tx0))
> > + return PTR_ERR(edp->tx0);
> > +
> > + edp->tx1 = devm_platform_ioremap_resource(pdev, 2);
> > + if (IS_ERR(edp->tx1))
> > + return PTR_ERR(edp->tx1);
> > +
> > + edp->pll = devm_platform_ioremap_resource(pdev, 3);
> > + if (IS_ERR(edp->pll))
> > + return PTR_ERR(edp->pll);
> > +
> > + edp->clks[0].id = "aux";
> > + edp->clks[1].id = "cfg_ahb";
> > + ret = devm_clk_bulk_get(dev, ARRAY_SIZE(edp->clks), edp->clks);
> > + if (ret)
> > + return ret;
> > +
> > + edp->supplies[0].supply = "vdda-phy";
> > + edp->supplies[1].supply = "vdda-pll";
> > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(edp->supplies),
> > edp->supplies);
> > + if (ret)
> > + return ret;
>
> I believe, the combination of the number of regulator and clk resources may
> vary based on the platform.
> I think we should not fail probe if all these resources are not present in
> the device tree file.
> I think, these resources can be optional. We can get these resources if they
> are present in the device tree file and enable them as required.
>
It's quite helpful to the DTS writer to actually encode in the driver
which resources the driver expects and provide useful error messages
when these expectations aren't met - so I think in line with most other
drivers this should be decided based on the compatible.
What clocks and regulators do you have on sc7280?
Thanks for the feedback, I see that I have a few more pieces of feedback
from others that I need to incorporate. I'll make sure to do that and
repost this patch shortly.
Regards,
Bjorn
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/2] phy: qcom: Introduce new eDP PHY driver
2021-08-10 3:36 ` Bjorn Andersson
@ 2021-08-12 0:23 ` sbillaka
0 siblings, 0 replies; 12+ messages in thread
From: sbillaka @ 2021-08-12 0:23 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Andy Gross, Kishon Vijay Abraham I, Vinod Koul, Rob Herring,
Rob Clark, Dmitry Baryshkov, Stephen Boyd, linux-arm-msm,
linux-phy, devicetree, linux-kernel, kalyan_t, abhinavk
On 2021-08-10 09:06, Bjorn Andersson wrote:
> On Mon 09 Aug 22:15 CDT 2021, sbillaka@codeaurora.org wrote:
>> On 2021-05-11 09:49, Bjorn Andersson wrote:
>> > diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c
> [..]
>> > +#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
>>
>> The DP_PHY_AUX_CFG0 offset for sc8180x eDP phy is 0x0024.
>> Some of the eDP PHY offset addresses are shifted by 4 address
>> locations,
>> compared to the DP QMP PHY offset addresses for sc8180x.
>> The DP_PHY_AUX_CFG* offsets for this eDP phy driver are as below:
>>
>> #define DP_PHY_AUX_CFG0 0x0024
>> #define DP_PHY_AUX_CFG1 0x0028
>> #define DP_PHY_AUX_CFG2 0x002c
>> #define DP_PHY_AUX_CFG3 0x0030
>> #define DP_PHY_AUX_CFG4 0x0034
>> #define DP_PHY_AUX_CFG5 0x0038
>> #define DP_PHY_AUX_CFG6 0x003c
>> #define DP_PHY_AUX_CFG7 0x0040
>> #define DP_PHY_AUX_CFG8 0x0044
>> #define DP_PHY_AUX_CFG9 0x0048
>>
>
> I noticed this as well. During development I just used the numbers
> directly in the code and I must have screwed up as I replaced them with
> defined - and somehow missed this in the testing before posting.
>
> Sorry about that.
>
> [..]
>> > +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;
>>
>> I think the number of clk and regulator resources can vary based on
>> platform.
>>
>
> If that's the case we should replace the ARRAY_SIZE() with an integer.
> But I prefer to wait with that until the number actually is variable.
>
> [..]
>> > +static int qcom_edp_phy_probe(struct platform_device *pdev)
>> > +{
>> > + struct phy_provider *phy_provider;
>> > + struct device *dev = &pdev->dev;
>> > + struct qcom_edp *edp;
>> > + int ret;
>> > +
>> > + edp = devm_kzalloc(dev, sizeof(*edp), GFP_KERNEL);
>> > + if (!edp)
>> > + return -ENOMEM;
>> > +
>> > + edp->dev = dev;
>> > +
>> > + edp->edp = devm_platform_ioremap_resource(pdev, 0);
>> > + if (IS_ERR(edp->edp))
>> > + return PTR_ERR(edp->edp);
>> > +
>> > + edp->tx0 = devm_platform_ioremap_resource(pdev, 1);
>> > + if (IS_ERR(edp->tx0))
>> > + return PTR_ERR(edp->tx0);
>> > +
>> > + edp->tx1 = devm_platform_ioremap_resource(pdev, 2);
>> > + if (IS_ERR(edp->tx1))
>> > + return PTR_ERR(edp->tx1);
>> > +
>> > + edp->pll = devm_platform_ioremap_resource(pdev, 3);
>> > + if (IS_ERR(edp->pll))
>> > + return PTR_ERR(edp->pll);
>> > +
>> > + edp->clks[0].id = "aux";
>> > + edp->clks[1].id = "cfg_ahb";
>> > + ret = devm_clk_bulk_get(dev, ARRAY_SIZE(edp->clks), edp->clks);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + edp->supplies[0].supply = "vdda-phy";
>> > + edp->supplies[1].supply = "vdda-pll";
>> > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(edp->supplies),
>> > edp->supplies);
>> > + if (ret)
>> > + return ret;
>>
>> I believe, the combination of the number of regulator and clk
>> resources may
>> vary based on the platform.
>> I think we should not fail probe if all these resources are not
>> present in
>> the device tree file.
>> I think, these resources can be optional. We can get these resources
>> if they
>> are present in the device tree file and enable them as required.
>>
>
> It's quite helpful to the DTS writer to actually encode in the driver
> which resources the driver expects and provide useful error messages
> when these expectations aren't met - so I think in line with most other
> drivers this should be decided based on the compatible.
>
> What clocks and regulators do you have on sc7280?
We have only one clock (edp refclk) and one regulator (phy-0p9) for
sc7280.
>
>
> Thanks for the feedback, I see that I have a few more pieces of
> feedback
> from others that I need to incorporate. I'll make sure to do that and
> repost this patch shortly.
Okay. Thank you.
>
> Regards,
> Bjorn
^ permalink raw reply [flat|nested] 12+ messages in thread