* [PATCH v2 1/2] phy/qcom: add regulator_set_load to edp/dp phy
2022-05-18 16:43 [PATCH v2 0/2] eDP/DP Phy vdda realted function Kuogee Hsieh
@ 2022-05-18 16:43 ` Kuogee Hsieh
2022-05-18 17:12 ` Dmitry Baryshkov
2022-05-18 16:43 ` [PATCH v2 2/2] drm/msm/dp: delete vdda regulator related functions from eDP/DP controller Kuogee Hsieh
2022-05-18 17:16 ` [PATCH v2 0/2] eDP/DP Phy vdda realted function Dmitry Baryshkov
2 siblings, 1 reply; 12+ messages in thread
From: Kuogee Hsieh @ 2022-05-18 16:43 UTC (permalink / raw)
To: dri-devel, robdclark, sean, swboyd, dianders, vkoul, daniel,
airlied, agross, dmitry.baryshkov, bjorn.andersson
Cc: Kuogee Hsieh, quic_abhinavk, quic_aravindh, quic_sbillaka,
freedreno, linux-arm-msm, linux-kernel
This patch add regulator_set_load() to both eDP and DP phy driver
to have totally control regulators.
Changes in v2:
-- no regulator_set_laod() before disable regulator
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
drivers/phy/qualcomm/phy-qcom-edp.c | 25 +++++++++++++++++++++----
drivers/phy/qualcomm/phy-qcom-qmp.c | 13 +++++++++++++
2 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
index cacd32f..9b55095 100644
--- a/drivers/phy/qualcomm/phy-qcom-edp.c
+++ b/drivers/phy/qualcomm/phy-qcom-edp.c
@@ -87,17 +87,24 @@ struct qcom_edp {
struct clk_bulk_data clks[2];
struct regulator_bulk_data supplies[2];
+ int enable_load[2];
+ int disable_load[2];
};
static int qcom_edp_phy_init(struct phy *phy)
{
struct qcom_edp *edp = phy_get_drvdata(phy);
int ret;
+ int num_consumers = ARRAY_SIZE(edp->supplies);
+ int i;
- ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies);
+ ret = regulator_bulk_enable(num_consumers, edp->supplies);
if (ret)
return ret;
+ for (i = num_consumers - 1; i >= 0; --i)
+ regulator_set_load(edp->supplies[i].consumer, edp->enable_load[i]);
+
ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks);
if (ret)
goto out_disable_supplies;
@@ -425,9 +432,15 @@ static int qcom_edp_phy_power_off(struct phy *phy)
static int qcom_edp_phy_exit(struct phy *phy)
{
struct qcom_edp *edp = phy_get_drvdata(phy);
+ int num_consumers = ARRAY_SIZE(edp->supplies);
+ int i;
clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks);
- regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies);
+
+ for (i = num_consumers - 1; i >= 0; --i)
+ regulator_set_load(edp->supplies[i].consumer, edp->disable_load[i]);
+
+ regulator_bulk_disable(num_consumers, edp->supplies);
return 0;
}
@@ -633,8 +646,12 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
if (ret)
return ret;
- edp->supplies[0].supply = "vdda-phy";
- edp->supplies[1].supply = "vdda-pll";
+ edp->supplies[0].supply = "vdda-1p2";
+ edp->supplies[1].supply = "vdda-0p9";
+ edp->enable_load[0] = 21800; /* 1.2 V */
+ edp->enable_load[1] = 36000; /* 1.2 V */
+ edp->disable_load[0] = 4; /* 0.9 V */
+ edp->disable_load[1] = 4; /* 10.9V */
ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(edp->supplies), edp->supplies);
if (ret)
return ret;
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index b144ae1..0a4c8a8 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -3130,6 +3130,7 @@ struct qmp_phy_cfg {
int num_resets;
/* regulators to be requested */
const char * const *vreg_list;
+ const unsigned int *vreg_enable_load;
int num_vregs;
/* array of registers with different offsets */
@@ -3346,6 +3347,10 @@ static const char * const qmp_phy_vreg_l[] = {
"vdda-phy", "vdda-pll",
};
+static const unsigned int qmp_phy_vreg_enable_load[] = {
+ 21800, 36000
+};
+
static const struct qmp_phy_cfg ipq8074_usb3phy_cfg = {
.type = PHY_TYPE_USB3,
.nlanes = 1,
@@ -4072,6 +4077,7 @@ static const struct qmp_phy_cfg sm8250_usb3phy_cfg = {
.reset_list = msm8996_usb3phy_reset_l,
.num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
.vreg_list = qmp_phy_vreg_l,
+ .vreg_enable_load = qmp_phy_vreg_enable_load,
.num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
.regs = qmp_v4_usb3phy_regs_layout,
@@ -4139,6 +4145,7 @@ static const struct qmp_phy_cfg sm8250_dpphy_cfg = {
.reset_list = msm8996_usb3phy_reset_l,
.num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
.vreg_list = qmp_phy_vreg_l,
+ .vreg_enable_load = qmp_phy_vreg_enable_load,
.num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
.regs = qmp_v4_usb3phy_regs_layout,
@@ -5008,6 +5015,11 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
return 0;
}
+ if (cfg->vreg_enable_load) {
+ for (i = cfg->num_vregs - 1; i >= 0; --i)
+ regulator_set_load(qmp->vregs[i].consumer, cfg->vreg_enable_load[i]);
+ }
+
/* turn on regulator supplies */
ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
if (ret) {
@@ -5116,6 +5128,7 @@ static int qcom_qmp_phy_com_exit(struct qmp_phy *qphy)
clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks);
+ /* no minimum load set required before disable regulator */
regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
mutex_unlock(&qmp->phy_mutex);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 1/2] phy/qcom: add regulator_set_load to edp/dp phy
2022-05-18 16:43 ` [PATCH v2 1/2] phy/qcom: add regulator_set_load to edp/dp phy Kuogee Hsieh
@ 2022-05-18 17:12 ` Dmitry Baryshkov
2022-05-18 17:36 ` Kuogee Hsieh
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Baryshkov @ 2022-05-18 17:12 UTC (permalink / raw)
To: Kuogee Hsieh
Cc: dri-devel, robdclark, sean, swboyd, dianders, vkoul, daniel,
airlied, agross, bjorn.andersson, quic_abhinavk, quic_aravindh,
quic_sbillaka, freedreno, linux-arm-msm, linux-kernel
On Wed, 18 May 2022 at 19:43, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> This patch add regulator_set_load() to both eDP and DP phy driver
> to have totally control regulators.
>
> Changes in v2:
> -- no regulator_set_laod() before disable regulator
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
> drivers/phy/qualcomm/phy-qcom-edp.c | 25 +++++++++++++++++++++----
> drivers/phy/qualcomm/phy-qcom-qmp.c | 13 +++++++++++++
Split into -edp and -qmp part.
> 2 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
> index cacd32f..9b55095 100644
> --- a/drivers/phy/qualcomm/phy-qcom-edp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-edp.c
> @@ -87,17 +87,24 @@ struct qcom_edp {
>
> struct clk_bulk_data clks[2];
> struct regulator_bulk_data supplies[2];
> + int enable_load[2];
> + int disable_load[2];
As noticed in the review of the previous patch, disable_load is unnecessary.
> };
>
> static int qcom_edp_phy_init(struct phy *phy)
> {
> struct qcom_edp *edp = phy_get_drvdata(phy);
> int ret;
> + int num_consumers = ARRAY_SIZE(edp->supplies);
> + int i;
>
> - ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies);
> + ret = regulator_bulk_enable(num_consumers, edp->supplies);
> if (ret)
> return ret;
>
> + for (i = num_consumers - 1; i >= 0; --i)
> + regulator_set_load(edp->supplies[i].consumer, edp->enable_load[i]);
> +
> ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks);
> if (ret)
> goto out_disable_supplies;
> @@ -425,9 +432,15 @@ static int qcom_edp_phy_power_off(struct phy *phy)
> static int qcom_edp_phy_exit(struct phy *phy)
> {
> struct qcom_edp *edp = phy_get_drvdata(phy);
> + int num_consumers = ARRAY_SIZE(edp->supplies);
> + int i;
>
> clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks);
> - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies);
> +
> + for (i = num_consumers - 1; i >= 0; --i)
> + regulator_set_load(edp->supplies[i].consumer, edp->disable_load[i]);
> +
> + regulator_bulk_disable(num_consumers, edp->supplies);
>
> return 0;
> }
> @@ -633,8 +646,12 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - edp->supplies[0].supply = "vdda-phy";
> - edp->supplies[1].supply = "vdda-pll";
> + edp->supplies[0].supply = "vdda-1p2";
> + edp->supplies[1].supply = "vdda-0p9";
NAK, You can not randomly change supply names.
> + edp->enable_load[0] = 21800; /* 1.2 V */
> + edp->enable_load[1] = 36000; /* 1.2 V */
> + edp->disable_load[0] = 4; /* 0.9 V */
> + edp->disable_load[1] = 4; /* 10.9V */
Again, 10.9V here. Kuogee. Have you read the review points?
> ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(edp->supplies), edp->supplies);
> if (ret)
> return ret;
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index b144ae1..0a4c8a8 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -3130,6 +3130,7 @@ struct qmp_phy_cfg {
> int num_resets;
> /* regulators to be requested */
> const char * const *vreg_list;
> + const unsigned int *vreg_enable_load;
> int num_vregs;
>
> /* array of registers with different offsets */
> @@ -3346,6 +3347,10 @@ static const char * const qmp_phy_vreg_l[] = {
> "vdda-phy", "vdda-pll",
> };
>
> +static const unsigned int qmp_phy_vreg_enable_load[] = {
> + 21800, 36000
> +};
> +
> static const struct qmp_phy_cfg ipq8074_usb3phy_cfg = {
> .type = PHY_TYPE_USB3,
> .nlanes = 1,
> @@ -4072,6 +4077,7 @@ static const struct qmp_phy_cfg sm8250_usb3phy_cfg = {
> .reset_list = msm8996_usb3phy_reset_l,
> .num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
> .vreg_list = qmp_phy_vreg_l,
> + .vreg_enable_load = qmp_phy_vreg_enable_load,
> .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
> .regs = qmp_v4_usb3phy_regs_layout,
>
> @@ -4139,6 +4145,7 @@ static const struct qmp_phy_cfg sm8250_dpphy_cfg = {
> .reset_list = msm8996_usb3phy_reset_l,
> .num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
> .vreg_list = qmp_phy_vreg_l,
> + .vreg_enable_load = qmp_phy_vreg_enable_load,
> .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
> .regs = qmp_v4_usb3phy_regs_layout,
>
> @@ -5008,6 +5015,11 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
> return 0;
> }
>
> + if (cfg->vreg_enable_load) {
> + for (i = cfg->num_vregs - 1; i >= 0; --i)
What's the point of iterating the list backwards?
> + regulator_set_load(qmp->vregs[i].consumer, cfg->vreg_enable_load[i]);
> + }
> +
> /* turn on regulator supplies */
> ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
> if (ret) {
> @@ -5116,6 +5128,7 @@ static int qcom_qmp_phy_com_exit(struct qmp_phy *qphy)
>
> clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks);
>
> + /* no minimum load set required before disable regulator */
No need for the comment.
> regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>
> mutex_unlock(&qmp->phy_mutex);
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 1/2] phy/qcom: add regulator_set_load to edp/dp phy
2022-05-18 17:12 ` Dmitry Baryshkov
@ 2022-05-18 17:36 ` Kuogee Hsieh
2022-05-18 17:52 ` Dmitry Baryshkov
0 siblings, 1 reply; 12+ messages in thread
From: Kuogee Hsieh @ 2022-05-18 17:36 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: dri-devel, robdclark, sean, swboyd, dianders, vkoul, daniel,
airlied, agross, bjorn.andersson, quic_abhinavk, quic_aravindh,
quic_sbillaka, freedreno, linux-arm-msm, linux-kernel
On 5/18/2022 10:12 AM, Dmitry Baryshkov wrote:
> On Wed, 18 May 2022 at 19:43, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> This patch add regulator_set_load() to both eDP and DP phy driver
>> to have totally control regulators.
>>
>> Changes in v2:
>> -- no regulator_set_laod() before disable regulator
>>
>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>> ---
>> drivers/phy/qualcomm/phy-qcom-edp.c | 25 +++++++++++++++++++++----
>> drivers/phy/qualcomm/phy-qcom-qmp.c | 13 +++++++++++++
> Split into -edp and -qmp part.
>
>> 2 files changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c b/drivers/phy/qualcomm/phy-qcom-edp.c
>> index cacd32f..9b55095 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-edp.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-edp.c
>> @@ -87,17 +87,24 @@ struct qcom_edp {
>>
>> struct clk_bulk_data clks[2];
>> struct regulator_bulk_data supplies[2];
>> + int enable_load[2];
>> + int disable_load[2];
> As noticed in the review of the previous patch, disable_load is unnecessary.
>
>> };
>>
>> static int qcom_edp_phy_init(struct phy *phy)
>> {
>> struct qcom_edp *edp = phy_get_drvdata(phy);
>> int ret;
>> + int num_consumers = ARRAY_SIZE(edp->supplies);
>> + int i;
>>
>> - ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies), edp->supplies);
>> + ret = regulator_bulk_enable(num_consumers, edp->supplies);
>> if (ret)
>> return ret;
>>
>> + for (i = num_consumers - 1; i >= 0; --i)
>> + regulator_set_load(edp->supplies[i].consumer, edp->enable_load[i]);
>> +
>> ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks), edp->clks);
>> if (ret)
>> goto out_disable_supplies;
>> @@ -425,9 +432,15 @@ static int qcom_edp_phy_power_off(struct phy *phy)
>> static int qcom_edp_phy_exit(struct phy *phy)
>> {
>> struct qcom_edp *edp = phy_get_drvdata(phy);
>> + int num_consumers = ARRAY_SIZE(edp->supplies);
>> + int i;
>>
>> clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks);
>> - regulator_bulk_disable(ARRAY_SIZE(edp->supplies), edp->supplies);
>> +
>> + for (i = num_consumers - 1; i >= 0; --i)
>> + regulator_set_load(edp->supplies[i].consumer, edp->disable_load[i]);
>> +
>> + regulator_bulk_disable(num_consumers, edp->supplies);
>>
>> return 0;
>> }
>> @@ -633,8 +646,12 @@ static int qcom_edp_phy_probe(struct platform_device *pdev)
>> if (ret)
>> return ret;
>>
>> - edp->supplies[0].supply = "vdda-phy";
>> - edp->supplies[1].supply = "vdda-pll";
>> + edp->supplies[0].supply = "vdda-1p2";
>> + edp->supplies[1].supply = "vdda-0p9";
> NAK, You can not randomly change supply names.
if you do no change here, then we have to change dtsi.
They are not match.
>
>> + edp->enable_load[0] = 21800; /* 1.2 V */
>> + edp->enable_load[1] = 36000; /* 1.2 V */
>> + edp->disable_load[0] = 4; /* 0.9 V */
>> + edp->disable_load[1] = 4; /* 10.9V */
> Again, 10.9V here. Kuogee. Have you read the review points?
I have read it. but forget to make change at edp file.
>
>> ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(edp->supplies), edp->supplies);
>> if (ret)
>> return ret;
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> index b144ae1..0a4c8a8 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
>> @@ -3130,6 +3130,7 @@ struct qmp_phy_cfg {
>> int num_resets;
>> /* regulators to be requested */
>> const char * const *vreg_list;
>> + const unsigned int *vreg_enable_load;
>> int num_vregs;
>>
>> /* array of registers with different offsets */
>> @@ -3346,6 +3347,10 @@ static const char * const qmp_phy_vreg_l[] = {
>> "vdda-phy", "vdda-pll",
>> };
>>
>> +static const unsigned int qmp_phy_vreg_enable_load[] = {
>> + 21800, 36000
>> +};
>> +
>> static const struct qmp_phy_cfg ipq8074_usb3phy_cfg = {
>> .type = PHY_TYPE_USB3,
>> .nlanes = 1,
>> @@ -4072,6 +4077,7 @@ static const struct qmp_phy_cfg sm8250_usb3phy_cfg = {
>> .reset_list = msm8996_usb3phy_reset_l,
>> .num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
>> .vreg_list = qmp_phy_vreg_l,
>> + .vreg_enable_load = qmp_phy_vreg_enable_load,
>> .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
>> .regs = qmp_v4_usb3phy_regs_layout,
>>
>> @@ -4139,6 +4145,7 @@ static const struct qmp_phy_cfg sm8250_dpphy_cfg = {
>> .reset_list = msm8996_usb3phy_reset_l,
>> .num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
>> .vreg_list = qmp_phy_vreg_l,
>> + .vreg_enable_load = qmp_phy_vreg_enable_load,
>> .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
>> .regs = qmp_v4_usb3phy_regs_layout,
>>
>> @@ -5008,6 +5015,11 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
>> return 0;
>> }
>>
>> + if (cfg->vreg_enable_load) {
>> + for (i = cfg->num_vregs - 1; i >= 0; --i)
> What's the point of iterating the list backwards?
do no know,
I just follow the order from regulator_bulk_enable()
>
>> + regulator_set_load(qmp->vregs[i].consumer, cfg->vreg_enable_load[i]);
>> + }
>> +
>> /* turn on regulator supplies */
>> ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>> if (ret) {
>> @@ -5116,6 +5128,7 @@ static int qcom_qmp_phy_com_exit(struct qmp_phy *qphy)
>>
>> clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks);
>>
>> + /* no minimum load set required before disable regulator */
> No otneed for the comment.
>
>> regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>>
>> mutex_unlock(&qmp->phy_mutex);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 1/2] phy/qcom: add regulator_set_load to edp/dp phy
2022-05-18 17:36 ` Kuogee Hsieh
@ 2022-05-18 17:52 ` Dmitry Baryshkov
2022-05-18 18:27 ` Kuogee Hsieh
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Baryshkov @ 2022-05-18 17:52 UTC (permalink / raw)
To: Kuogee Hsieh
Cc: dri-devel, robdclark, sean, swboyd, dianders, vkoul, daniel,
airlied, agross, bjorn.andersson, quic_abhinavk, quic_aravindh,
quic_sbillaka, freedreno, linux-arm-msm, linux-kernel
On 18/05/2022 20:36, Kuogee Hsieh wrote:
>
> On 5/18/2022 10:12 AM, Dmitry Baryshkov wrote:
>> On Wed, 18 May 2022 at 19:43, Kuogee Hsieh <quic_khsieh@quicinc.com>
>> wrote:
>>> This patch add regulator_set_load() to both eDP and DP phy driver
>>> to have totally control regulators.
>>>
>>> Changes in v2:
>>> -- no regulator_set_laod() before disable regulator
>>>
>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> ---
>>> drivers/phy/qualcomm/phy-qcom-edp.c | 25 +++++++++++++++++++++----
>>> drivers/phy/qualcomm/phy-qcom-qmp.c | 13 +++++++++++++
>> Split into -edp and -qmp part.
>>
>>> 2 files changed, 34 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c
>>> b/drivers/phy/qualcomm/phy-qcom-edp.c
>>> index cacd32f..9b55095 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-edp.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-edp.c
>>> @@ -87,17 +87,24 @@ struct qcom_edp {
>>>
>>> struct clk_bulk_data clks[2];
>>> struct regulator_bulk_data supplies[2];
>>> + int enable_load[2];
>>> + int disable_load[2];
>> As noticed in the review of the previous patch, disable_load is
>> unnecessary.
>>
>>> };
>>>
>>> static int qcom_edp_phy_init(struct phy *phy)
>>> {
>>> struct qcom_edp *edp = phy_get_drvdata(phy);
>>> int ret;
>>> + int num_consumers = ARRAY_SIZE(edp->supplies);
>>> + int i;
>>>
>>> - ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies),
>>> edp->supplies);
>>> + ret = regulator_bulk_enable(num_consumers, edp->supplies);
>>> if (ret)
>>> return ret;
>>>
>>> + for (i = num_consumers - 1; i >= 0; --i)
>>> + regulator_set_load(edp->supplies[i].consumer,
>>> edp->enable_load[i]);
>>> +
>>> ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks),
>>> edp->clks);
>>> if (ret)
>>> goto out_disable_supplies;
>>> @@ -425,9 +432,15 @@ static int qcom_edp_phy_power_off(struct phy *phy)
>>> static int qcom_edp_phy_exit(struct phy *phy)
>>> {
>>> struct qcom_edp *edp = phy_get_drvdata(phy);
>>> + int num_consumers = ARRAY_SIZE(edp->supplies);
>>> + int i;
>>>
>>> clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks);
>>> - regulator_bulk_disable(ARRAY_SIZE(edp->supplies),
>>> edp->supplies);
>>> +
>>> + for (i = num_consumers - 1; i >= 0; --i)
>>> + regulator_set_load(edp->supplies[i].consumer,
>>> edp->disable_load[i]);
>>> +
>>> + regulator_bulk_disable(num_consumers, edp->supplies);
>>>
>>> return 0;
>>> }
>>> @@ -633,8 +646,12 @@ static int qcom_edp_phy_probe(struct
>>> platform_device *pdev)
>>> if (ret)
>>> return ret;
>>>
>>> - edp->supplies[0].supply = "vdda-phy";
>>> - edp->supplies[1].supply = "vdda-pll";
>>> + edp->supplies[0].supply = "vdda-1p2";
>>> + edp->supplies[1].supply = "vdda-0p9";
>> NAK, You can not randomly change supply names.
>
> if you do no change here, then we have to change dtsi.
>
> They are not match.
Where is no match? I don't see any in-kernel dtsi using them.
>>> + edp->enable_load[0] = 21800; /* 1.2 V */
>>> + edp->enable_load[1] = 36000; /* 1.2 V */
>>> + edp->disable_load[0] = 4; /* 0.9 V */
>>> + edp->disable_load[1] = 4; /* 10.9V */
>> Again, 10.9V here. Kuogee. Have you read the review points?
> I have read it. but forget to make change at edp file.
>>
>>> ret = devm_regulator_bulk_get(dev,
>>> ARRAY_SIZE(edp->supplies), edp->supplies);
>>> if (ret)
>>> return ret;
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
>>> b/drivers/phy/qualcomm/phy-qcom-qmp.c
>>> index b144ae1..0a4c8a8 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
>>> @@ -3130,6 +3130,7 @@ struct qmp_phy_cfg {
>>> int num_resets;
>>> /* regulators to be requested */
>>> const char * const *vreg_list;
>>> + const unsigned int *vreg_enable_load;
>>> int num_vregs;
>>>
>>> /* array of registers with different offsets */
>>> @@ -3346,6 +3347,10 @@ static const char * const qmp_phy_vreg_l[] = {
>>> "vdda-phy", "vdda-pll",
>>> };
>>>
>>> +static const unsigned int qmp_phy_vreg_enable_load[] = {
>>> + 21800, 36000
>>> +};
>>> +
>>> static const struct qmp_phy_cfg ipq8074_usb3phy_cfg = {
>>> .type = PHY_TYPE_USB3,
>>> .nlanes = 1,
>>> @@ -4072,6 +4077,7 @@ static const struct qmp_phy_cfg
>>> sm8250_usb3phy_cfg = {
>>> .reset_list = msm8996_usb3phy_reset_l,
>>> .num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
>>> .vreg_list = qmp_phy_vreg_l,
>>> + .vreg_enable_load = qmp_phy_vreg_enable_load,
>>> .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
>>> .regs = qmp_v4_usb3phy_regs_layout,
>>>
>>> @@ -4139,6 +4145,7 @@ static const struct qmp_phy_cfg
>>> sm8250_dpphy_cfg = {
>>> .reset_list = msm8996_usb3phy_reset_l,
>>> .num_resets = ARRAY_SIZE(msm8996_usb3phy_reset_l),
>>> .vreg_list = qmp_phy_vreg_l,
>>> + .vreg_enable_load = qmp_phy_vreg_enable_load,
So, you apply this change only to the sm8250 (sc7280) config. Are you
sure that both of them have the same requirement?
Also there are other DP phy instances (sc8180x, sc7180). Do they have to
be extended too?
>>> .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
>>> .regs = qmp_v4_usb3phy_regs_layout,
>>>
>>> @@ -5008,6 +5015,11 @@ static int qcom_qmp_phy_com_init(struct
>>> qmp_phy *qphy)
>>> return 0;
>>> }
>>>
>>> + if (cfg->vreg_enable_load) {
>>> + for (i = cfg->num_vregs - 1; i >= 0; --i)
>> What's the point of iterating the list backwards?
>
> do no know,
>
> I just follow the order from regulator_bulk_enable()
regulator_bulk_enable() iterates the list in the ascending order.
>
>>
>>> + regulator_set_load(qmp->vregs[i].consumer,
>>> cfg->vreg_enable_load[i]);
>>> + }
>>> +
>>> /* turn on regulator supplies */
>>> ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>>> if (ret) {
>>> @@ -5116,6 +5128,7 @@ static int qcom_qmp_phy_com_exit(struct qmp_phy
>>> *qphy)
>>>
>>> clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks);
>>>
>>> + /* no minimum load set required before disable regulator */
>> No otneed for the comment.
>>
>>> regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>>>
>>> mutex_unlock(&qmp->phy_mutex);
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>> Forum,
>>> a Linux Foundation Collaborative Project
>>>
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 1/2] phy/qcom: add regulator_set_load to edp/dp phy
2022-05-18 17:52 ` Dmitry Baryshkov
@ 2022-05-18 18:27 ` Kuogee Hsieh
0 siblings, 0 replies; 12+ messages in thread
From: Kuogee Hsieh @ 2022-05-18 18:27 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: dri-devel, robdclark, sean, swboyd, dianders, vkoul, daniel,
airlied, agross, bjorn.andersson, quic_abhinavk, quic_aravindh,
quic_sbillaka, freedreno, linux-arm-msm, linux-kernel
On 5/18/2022 10:52 AM, Dmitry Baryshkov wrote:
> On 18/05/2022 20:36, Kuogee Hsieh wrote:
>>
>> On 5/18/2022 10:12 AM, Dmitry Baryshkov wrote:
>>> On Wed, 18 May 2022 at 19:43, Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> wrote:
>>>> This patch add regulator_set_load() to both eDP and DP phy driver
>>>> to have totally control regulators.
>>>>
>>>> Changes in v2:
>>>> -- no regulator_set_laod() before disable regulator
>>>>
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> ---
>>>> drivers/phy/qualcomm/phy-qcom-edp.c | 25 +++++++++++++++++++++----
>>>> drivers/phy/qualcomm/phy-qcom-qmp.c | 13 +++++++++++++
>>> Split into -edp and -qmp part.
>>>
>>>> 2 files changed, 34 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-edp.c
>>>> b/drivers/phy/qualcomm/phy-qcom-edp.c
>>>> index cacd32f..9b55095 100644
>>>> --- a/drivers/phy/qualcomm/phy-qcom-edp.c
>>>> +++ b/drivers/phy/qualcomm/phy-qcom-edp.c
>>>> @@ -87,17 +87,24 @@ struct qcom_edp {
>>>>
>>>> struct clk_bulk_data clks[2];
>>>> struct regulator_bulk_data supplies[2];
>>>> + int enable_load[2];
>>>> + int disable_load[2];
>>> As noticed in the review of the previous patch, disable_load is
>>> unnecessary.
>>>
>>>> };
>>>>
>>>> static int qcom_edp_phy_init(struct phy *phy)
>>>> {
>>>> struct qcom_edp *edp = phy_get_drvdata(phy);
>>>> int ret;
>>>> + int num_consumers = ARRAY_SIZE(edp->supplies);
>>>> + int i;
>>>>
>>>> - ret = regulator_bulk_enable(ARRAY_SIZE(edp->supplies),
>>>> edp->supplies);
>>>> + ret = regulator_bulk_enable(num_consumers, edp->supplies);
>>>> if (ret)
>>>> return ret;
>>>>
>>>> + for (i = num_consumers - 1; i >= 0; --i)
>>>> + regulator_set_load(edp->supplies[i].consumer, edp->enable_load[i]);
>>>> +
>>>> ret = clk_bulk_prepare_enable(ARRAY_SIZE(edp->clks),
>>>> edp->clks);
>>>> if (ret)
>>>> goto out_disable_supplies;
>>>> @@ -425,9 +432,15 @@ static int qcom_edp_phy_power_off(struct phy
>>>> *phy)
>>>> static int qcom_edp_phy_exit(struct phy *phy)
>>>> {
>>>> struct qcom_edp *edp = phy_get_drvdata(phy);
>>>> + int num_consumers = ARRAY_SIZE(edp->supplies);
>>>> + int i;
>>>>
>>>> clk_bulk_disable_unprepare(ARRAY_SIZE(edp->clks), edp->clks);
>>>> - regulator_bulk_disable(ARRAY_SIZE(edp->supplies),
>>>> edp->supplies);
>>>> +
>>>> + for (i = num_consumers - 1; i >= 0; --i)
>>>> + regulator_set_load(edp->supplies[i].consumer, edp->disable_load[i]);
>>>> +
>>>> + regulator_bulk_disable(num_consumers, edp->supplies);
>>>>
>>>> return 0;
>>>> }
>>>> @@ -633,8 +646,12 @@ static int qcom_edp_phy_probe(struct
>>>> platform_device *pdev)
>>>> if (ret)
>>>> return ret;
>>>>
>>>> - edp->supplies[0].supply = "vdda-phy";
>>>> - edp->supplies[1].supply = "vdda-pll";
>>>> + edp->supplies[0].supply = "vdda-1p2";
>>>> + edp->supplies[1].supply = "vdda-0p9";
>>> NAK, You can not randomly change supply names.
>>
>> if you do no change here, then we have to change dtsi.
>>
>> They are not match.
>
> Where is no match? I don't see any in-kernel dtsi using them.
my mistake, we did not pull in Doug's patch at our internal release
where i run my test.
>
>
>>>> + edp->enable_load[0] = 21800; /* 1.2 V */
>>>> + edp->enable_load[1] = 36000; /* 1.2 V */
>>>> + edp->disable_load[0] = 4; /* 0.9 V */
>>>> + edp->disable_load[1] = 4; /* 10.9V */
>>> Again, 10.9V here. Kuogee. Have you read the review points?
>> I have read it. but forget to make change at edp file.
>>>
>>>> ret = devm_regulator_bulk_get(dev,
>>>> ARRAY_SIZE(edp->supplies), edp->supplies);
>>>> if (ret)
>>>> return ret;
>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
>>>> b/drivers/phy/qualcomm/phy-qcom-qmp.c
>>>> index b144ae1..0a4c8a8 100644
>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
>>>> @@ -3130,6 +3130,7 @@ struct qmp_phy_cfg {
>>>> int num_resets;
>>>> /* regulators to be requested */
>>>> const char * const *vreg_list;
>>>> + const unsigned int *vreg_enable_load;
>>>> int num_vregs;
>>>>
>>>> /* array of registers with different offsets */
>>>> @@ -3346,6 +3347,10 @@ static const char * const qmp_phy_vreg_l[] = {
>>>> "vdda-phy", "vdda-pll",
>>>> };
>>>>
>>>> +static const unsigned int qmp_phy_vreg_enable_load[] = {
>>>> + 21800, 36000
>>>> +};
>>>> +
>>>> static const struct qmp_phy_cfg ipq8074_usb3phy_cfg = {
>>>> .type = PHY_TYPE_USB3,
>>>> .nlanes = 1,
>>>> @@ -4072,6 +4077,7 @@ static const struct qmp_phy_cfg
>>>> sm8250_usb3phy_cfg = {
>>>> .reset_list = msm8996_usb3phy_reset_l,
>>>> .num_resets =
>>>> ARRAY_SIZE(msm8996_usb3phy_reset_l),
>>>> .vreg_list = qmp_phy_vreg_l,
>>>> + .vreg_enable_load = qmp_phy_vreg_enable_load,
>>>> .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
>>>> .regs = qmp_v4_usb3phy_regs_layout,
>>>>
>>>> @@ -4139,6 +4145,7 @@ static const struct qmp_phy_cfg
>>>> sm8250_dpphy_cfg = {
>>>> .reset_list = msm8996_usb3phy_reset_l,
>>>> .num_resets =
>>>> ARRAY_SIZE(msm8996_usb3phy_reset_l),
>>>> .vreg_list = qmp_phy_vreg_l,
>>>> + .vreg_enable_load = qmp_phy_vreg_enable_load,
>
> So, you apply this change only to the sm8250 (sc7280) config. Are you
> sure that both of them have the same requirement?
>
> Also there are other DP phy instances (sc8180x, sc7180). Do they have
> to be extended too?
>
>>>> .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l),
>>>> .regs = qmp_v4_usb3phy_regs_layout,
>>>>
>>>> @@ -5008,6 +5015,11 @@ static int qcom_qmp_phy_com_init(struct
>>>> qmp_phy *qphy)
>>>> return 0;
>>>> }
>>>>
>>>> + if (cfg->vreg_enable_load) {
>>>> + for (i = cfg->num_vregs - 1; i >= 0; --i)
>>> What's the point of iterating the list backwards?
>>
>> do no know,
>>
>> I just follow the order from regulator_bulk_enable()
>
> regulator_bulk_enable() iterates the list in the ascending order.
>
>>
>>>
>>>> + regulator_set_load(qmp->vregs[i].consumer,
>>>> cfg->vreg_enable_load[i]);
>>>> + }
>>>> +
>>>> /* turn on regulator supplies */
>>>> ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
>>>> if (ret) {
>>>> @@ -5116,6 +5128,7 @@ static int qcom_qmp_phy_com_exit(struct
>>>> qmp_phy *qphy)
>>>>
>>>> clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks);
>>>>
>>>> + /* no minimum load set required before disable regulator */
>>> No otneed for the comment.
>>>
>>>> regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
>>>>
>>>> mutex_unlock(&qmp->phy_mutex);
>>>> --
>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>>> Forum,
>>>> a Linux Foundation Collaborative Project
>>>>
>>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] drm/msm/dp: delete vdda regulator related functions from eDP/DP controller
2022-05-18 16:43 [PATCH v2 0/2] eDP/DP Phy vdda realted function Kuogee Hsieh
2022-05-18 16:43 ` [PATCH v2 1/2] phy/qcom: add regulator_set_load to edp/dp phy Kuogee Hsieh
@ 2022-05-18 16:43 ` Kuogee Hsieh
2022-05-18 17:13 ` Dmitry Baryshkov
2022-05-18 17:16 ` [PATCH v2 0/2] eDP/DP Phy vdda realted function Dmitry Baryshkov
2 siblings, 1 reply; 12+ messages in thread
From: Kuogee Hsieh @ 2022-05-18 16:43 UTC (permalink / raw)
To: dri-devel, robdclark, sean, swboyd, dianders, vkoul, daniel,
airlied, agross, dmitry.baryshkov, bjorn.andersson
Cc: Kuogee Hsieh, quic_abhinavk, quic_aravindh, quic_sbillaka,
freedreno, linux-arm-msm, linux-kernel
Vdda regulators are related to both eDP and DP phy so that it should be
managed at eDP and DP phy driver instead of controller. This patch remove
vdda regulators related functions out of eDP/DP controller.
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
drivers/gpu/drm/msm/dp/dp_parser.c | 14 ------
drivers/gpu/drm/msm/dp/dp_parser.h | 6 ---
drivers/gpu/drm/msm/dp/dp_power.c | 95 +-------------------------------------
3 files changed, 2 insertions(+), 113 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
index 8f9fed9..4ef2130 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.c
+++ b/drivers/gpu/drm/msm/dp/dp_parser.c
@@ -22,14 +22,6 @@
#define DP_DEFAULT_P0_OFFSET 0x1000
#define DP_DEFAULT_P0_SIZE 0x0400
-static const struct dp_regulator_cfg sdm845_dp_reg_cfg = {
- .num = 2,
- .regs = {
- {"vdda-1p2", 21800, 4 }, /* 1.2 V */
- {"vdda-0p9", 36000, 32 }, /* 0.9 V */
- },
-};
-
static void __iomem *dp_ioremap(struct platform_device *pdev, int idx, size_t *len)
{
struct resource *res;
@@ -298,12 +290,6 @@ static int dp_parser_parse(struct dp_parser *parser)
if (rc)
return rc;
- /* Map the corresponding regulator information according to
- * version. Currently, since we only have one supported platform,
- * mapping the regulator directly.
- */
- parser->regulator_cfg = &sdm845_dp_reg_cfg;
-
return 0;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
index 3a4d797..b56b4d7 100644
--- a/drivers/gpu/drm/msm/dp/dp_parser.h
+++ b/drivers/gpu/drm/msm/dp/dp_parser.h
@@ -101,11 +101,6 @@ struct dp_reg_entry {
int disable_load;
};
-struct dp_regulator_cfg {
- int num;
- struct dp_reg_entry regs[DP_DEV_REGULATOR_MAX];
-};
-
/**
* struct dp_parser - DP parser's data exposed to clients
*
@@ -121,7 +116,6 @@ struct dp_parser {
struct dp_pinctrl pinctrl;
struct dp_io io;
struct dp_display_data disp_data;
- const struct dp_regulator_cfg *regulator_cfg;
u32 max_dp_lanes;
struct drm_bridge *next_bridge;
diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
index d9e0117..b52ac1d 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -20,82 +20,10 @@ struct dp_power_private {
struct clk *link_clk_src;
struct clk *pixel_provider;
struct clk *link_provider;
- struct regulator_bulk_data supplies[DP_DEV_REGULATOR_MAX];
struct dp_power dp_power;
};
-static void dp_power_regulator_disable(struct dp_power_private *power)
-{
- struct regulator_bulk_data *s = power->supplies;
- const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
- int num = power->parser->regulator_cfg->num;
- int i;
-
- DBG("");
- for (i = num - 1; i >= 0; i--)
- if (regs[i].disable_load >= 0)
- regulator_set_load(s[i].consumer,
- regs[i].disable_load);
-
- regulator_bulk_disable(num, s);
-}
-
-static int dp_power_regulator_enable(struct dp_power_private *power)
-{
- struct regulator_bulk_data *s = power->supplies;
- const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
- int num = power->parser->regulator_cfg->num;
- int ret, i;
-
- DBG("");
- for (i = 0; i < num; i++) {
- if (regs[i].enable_load >= 0) {
- ret = regulator_set_load(s[i].consumer,
- regs[i].enable_load);
- if (ret < 0) {
- pr_err("regulator %d set op mode failed, %d\n",
- i, ret);
- goto fail;
- }
- }
- }
-
- ret = regulator_bulk_enable(num, s);
- if (ret < 0) {
- pr_err("regulator enable failed, %d\n", ret);
- goto fail;
- }
-
- return 0;
-
-fail:
- for (i--; i >= 0; i--)
- regulator_set_load(s[i].consumer, regs[i].disable_load);
- return ret;
-}
-
-static int dp_power_regulator_init(struct dp_power_private *power)
-{
- struct regulator_bulk_data *s = power->supplies;
- const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
- struct platform_device *pdev = power->pdev;
- int num = power->parser->regulator_cfg->num;
- int i, ret;
-
- for (i = 0; i < num; i++)
- s[i].supply = regs[i].name;
-
- ret = devm_regulator_bulk_get(&pdev->dev, num, s);
- if (ret < 0) {
- pr_err("%s: failed to init regulator, ret=%d\n",
- __func__, ret);
- return ret;
- }
-
- return 0;
-}
-
static int dp_power_clk_init(struct dp_power_private *power)
{
int rc = 0;
@@ -318,21 +246,10 @@ int dp_power_client_init(struct dp_power *dp_power)
pm_runtime_enable(&power->pdev->dev);
- rc = dp_power_regulator_init(power);
- if (rc) {
- DRM_ERROR("failed to init regulators %d\n", rc);
- goto error;
- }
-
rc = dp_power_clk_init(power);
- if (rc) {
+ if (rc)
DRM_ERROR("failed to init clocks %d\n", rc);
- goto error;
- }
- return 0;
-error:
- pm_runtime_disable(&power->pdev->dev);
return rc;
}
@@ -365,22 +282,15 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
power = container_of(dp_power, struct dp_power_private, dp_power);
pm_runtime_get_sync(&power->pdev->dev);
- rc = dp_power_regulator_enable(power);
- if (rc) {
- DRM_ERROR("failed to enable regulators, %d\n", rc);
- goto exit;
- }
rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
if (rc) {
DRM_ERROR("failed to enable DP core clocks, %d\n", rc);
- goto err_clk;
+ goto exit;
}
return 0;
-err_clk:
- dp_power_regulator_disable(power);
exit:
pm_runtime_put_sync(&power->pdev->dev);
return rc;
@@ -393,7 +303,6 @@ int dp_power_deinit(struct dp_power *dp_power)
power = container_of(dp_power, struct dp_power_private, dp_power);
dp_power_clk_enable(dp_power, DP_CORE_PM, false);
- dp_power_regulator_disable(power);
pm_runtime_put_sync(&power->pdev->dev);
return 0;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/2] drm/msm/dp: delete vdda regulator related functions from eDP/DP controller
2022-05-18 16:43 ` [PATCH v2 2/2] drm/msm/dp: delete vdda regulator related functions from eDP/DP controller Kuogee Hsieh
@ 2022-05-18 17:13 ` Dmitry Baryshkov
0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Baryshkov @ 2022-05-18 17:13 UTC (permalink / raw)
To: Kuogee Hsieh
Cc: dri-devel, robdclark, sean, swboyd, dianders, vkoul, daniel,
airlied, agross, bjorn.andersson, quic_abhinavk, quic_aravindh,
quic_sbillaka, freedreno, linux-arm-msm, linux-kernel
On Wed, 18 May 2022 at 19:43, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> Vdda regulators are related to both eDP and DP phy so that it should be
> managed at eDP and DP phy driver instead of controller. This patch remove
removes
> vdda regulators related functions out of eDP/DP controller.
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
> drivers/gpu/drm/msm/dp/dp_parser.c | 14 ------
> drivers/gpu/drm/msm/dp/dp_parser.h | 6 ---
> drivers/gpu/drm/msm/dp/dp_power.c | 95 +-------------------------------------
> 3 files changed, 2 insertions(+), 113 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.c b/drivers/gpu/drm/msm/dp/dp_parser.c
> index 8f9fed9..4ef2130 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.c
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.c
> @@ -22,14 +22,6 @@
> #define DP_DEFAULT_P0_OFFSET 0x1000
> #define DP_DEFAULT_P0_SIZE 0x0400
>
> -static const struct dp_regulator_cfg sdm845_dp_reg_cfg = {
> - .num = 2,
> - .regs = {
> - {"vdda-1p2", 21800, 4 }, /* 1.2 V */
> - {"vdda-0p9", 36000, 32 }, /* 0.9 V */
> - },
> -};
> -
> static void __iomem *dp_ioremap(struct platform_device *pdev, int idx, size_t *len)
> {
> struct resource *res;
> @@ -298,12 +290,6 @@ static int dp_parser_parse(struct dp_parser *parser)
> if (rc)
> return rc;
>
> - /* Map the corresponding regulator information according to
> - * version. Currently, since we only have one supported platform,
> - * mapping the regulator directly.
> - */
> - parser->regulator_cfg = &sdm845_dp_reg_cfg;
> -
> return 0;
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h
> index 3a4d797..b56b4d7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
> @@ -101,11 +101,6 @@ struct dp_reg_entry {
> int disable_load;
> };
>
> -struct dp_regulator_cfg {
> - int num;
> - struct dp_reg_entry regs[DP_DEV_REGULATOR_MAX];
> -};
> -
> /**
> * struct dp_parser - DP parser's data exposed to clients
> *
> @@ -121,7 +116,6 @@ struct dp_parser {
> struct dp_pinctrl pinctrl;
> struct dp_io io;
> struct dp_display_data disp_data;
> - const struct dp_regulator_cfg *regulator_cfg;
> u32 max_dp_lanes;
> struct drm_bridge *next_bridge;
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index d9e0117..b52ac1d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -20,82 +20,10 @@ struct dp_power_private {
> struct clk *link_clk_src;
> struct clk *pixel_provider;
> struct clk *link_provider;
> - struct regulator_bulk_data supplies[DP_DEV_REGULATOR_MAX];
>
> struct dp_power dp_power;
> };
>
> -static void dp_power_regulator_disable(struct dp_power_private *power)
> -{
> - struct regulator_bulk_data *s = power->supplies;
> - const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
> - int num = power->parser->regulator_cfg->num;
> - int i;
> -
> - DBG("");
> - for (i = num - 1; i >= 0; i--)
> - if (regs[i].disable_load >= 0)
> - regulator_set_load(s[i].consumer,
> - regs[i].disable_load);
> -
> - regulator_bulk_disable(num, s);
> -}
> -
> -static int dp_power_regulator_enable(struct dp_power_private *power)
> -{
> - struct regulator_bulk_data *s = power->supplies;
> - const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
> - int num = power->parser->regulator_cfg->num;
> - int ret, i;
> -
> - DBG("");
> - for (i = 0; i < num; i++) {
> - if (regs[i].enable_load >= 0) {
> - ret = regulator_set_load(s[i].consumer,
> - regs[i].enable_load);
> - if (ret < 0) {
> - pr_err("regulator %d set op mode failed, %d\n",
> - i, ret);
> - goto fail;
> - }
> - }
> - }
> -
> - ret = regulator_bulk_enable(num, s);
> - if (ret < 0) {
> - pr_err("regulator enable failed, %d\n", ret);
> - goto fail;
> - }
> -
> - return 0;
> -
> -fail:
> - for (i--; i >= 0; i--)
> - regulator_set_load(s[i].consumer, regs[i].disable_load);
> - return ret;
> -}
> -
> -static int dp_power_regulator_init(struct dp_power_private *power)
> -{
> - struct regulator_bulk_data *s = power->supplies;
> - const struct dp_reg_entry *regs = power->parser->regulator_cfg->regs;
> - struct platform_device *pdev = power->pdev;
> - int num = power->parser->regulator_cfg->num;
> - int i, ret;
> -
> - for (i = 0; i < num; i++)
> - s[i].supply = regs[i].name;
> -
> - ret = devm_regulator_bulk_get(&pdev->dev, num, s);
> - if (ret < 0) {
> - pr_err("%s: failed to init regulator, ret=%d\n",
> - __func__, ret);
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> static int dp_power_clk_init(struct dp_power_private *power)
> {
> int rc = 0;
> @@ -318,21 +246,10 @@ int dp_power_client_init(struct dp_power *dp_power)
>
> pm_runtime_enable(&power->pdev->dev);
>
> - rc = dp_power_regulator_init(power);
> - if (rc) {
> - DRM_ERROR("failed to init regulators %d\n", rc);
> - goto error;
> - }
> -
> rc = dp_power_clk_init(power);
> - if (rc) {
> + if (rc)
> DRM_ERROR("failed to init clocks %d\n", rc);
> - goto error;
> - }
> - return 0;
>
> -error:
> - pm_runtime_disable(&power->pdev->dev);
> return rc;
> }
>
> @@ -365,22 +282,15 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
> power = container_of(dp_power, struct dp_power_private, dp_power);
>
> pm_runtime_get_sync(&power->pdev->dev);
> - rc = dp_power_regulator_enable(power);
> - if (rc) {
> - DRM_ERROR("failed to enable regulators, %d\n", rc);
> - goto exit;
> - }
>
> rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
> if (rc) {
> DRM_ERROR("failed to enable DP core clocks, %d\n", rc);
> - goto err_clk;
> + goto exit;
> }
>
> return 0;
>
> -err_clk:
> - dp_power_regulator_disable(power);
> exit:
> pm_runtime_put_sync(&power->pdev->dev);
> return rc;
> @@ -393,7 +303,6 @@ int dp_power_deinit(struct dp_power *dp_power)
> power = container_of(dp_power, struct dp_power_private, dp_power);
>
> dp_power_clk_enable(dp_power, DP_CORE_PM, false);
> - dp_power_regulator_disable(power);
> pm_runtime_put_sync(&power->pdev->dev);
> return 0;
> }
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/2] eDP/DP Phy vdda realted function
2022-05-18 16:43 [PATCH v2 0/2] eDP/DP Phy vdda realted function Kuogee Hsieh
2022-05-18 16:43 ` [PATCH v2 1/2] phy/qcom: add regulator_set_load to edp/dp phy Kuogee Hsieh
2022-05-18 16:43 ` [PATCH v2 2/2] drm/msm/dp: delete vdda regulator related functions from eDP/DP controller Kuogee Hsieh
@ 2022-05-18 17:16 ` Dmitry Baryshkov
2022-05-18 17:29 ` Kuogee Hsieh
2 siblings, 1 reply; 12+ messages in thread
From: Dmitry Baryshkov @ 2022-05-18 17:16 UTC (permalink / raw)
To: Kuogee Hsieh
Cc: dri-devel, robdclark, sean, swboyd, dianders, vkoul, daniel,
airlied, agross, bjorn.andersson, quic_abhinavk, quic_aravindh,
quic_sbillaka, freedreno, linux-arm-msm, linux-kernel
On Wed, 18 May 2022 at 19:43, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> 1) add regulator_set_load() to eDP/DP phy
> 2) remove vdda related function out of eDP/DP controller
These patches touch two subsystems and have a dependency between them.
How do we merge them?
>
> Kuogee Hsieh (2):
> phy/qcom: add regulator_set_load to edp/dp phy
> drm/msm/dp: delete vdda regulator related functions from eDP/DP
> controller
>
> drivers/gpu/drm/msm/dp/dp_parser.c | 14 ------
> drivers/gpu/drm/msm/dp/dp_parser.h | 6 ---
> drivers/gpu/drm/msm/dp/dp_power.c | 95 +------------------------------------
> drivers/phy/qualcomm/phy-qcom-edp.c | 25 ++++++++--
> drivers/phy/qualcomm/phy-qcom-qmp.c | 13 +++++
> 5 files changed, 36 insertions(+), 117 deletions(-)
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/2] eDP/DP Phy vdda realted function
2022-05-18 17:16 ` [PATCH v2 0/2] eDP/DP Phy vdda realted function Dmitry Baryshkov
@ 2022-05-18 17:29 ` Kuogee Hsieh
2022-05-18 17:31 ` Dmitry Baryshkov
0 siblings, 1 reply; 12+ messages in thread
From: Kuogee Hsieh @ 2022-05-18 17:29 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: dri-devel, robdclark, sean, swboyd, dianders, vkoul, daniel,
airlied, agross, bjorn.andersson, quic_abhinavk, quic_aravindh,
quic_sbillaka, freedreno, linux-arm-msm, linux-kernel
On 5/18/2022 10:16 AM, Dmitry Baryshkov wrote:
> On Wed, 18 May 2022 at 19:43, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>> 1) add regulator_set_load() to eDP/DP phy
>> 2) remove vdda related function out of eDP/DP controller
> These patches touch two subsystems and have a dependency between them.
> How do we merge them?
currently, both phy and controller are vote for regulator. The last vote
will just increase count.
Therefore the dependency should be very loose.
>> Kuogee Hsieh (2):
>> phy/qcom: add regulator_set_load to edp/dp phy
>> drm/msm/dp: delete vdda regulator related functions from eDP/DP
>> controller
>>
>> drivers/gpu/drm/msm/dp/dp_parser.c | 14 ------
>> drivers/gpu/drm/msm/dp/dp_parser.h | 6 ---
>> drivers/gpu/drm/msm/dp/dp_power.c | 95 +------------------------------------
>> drivers/phy/qualcomm/phy-qcom-edp.c | 25 ++++++++--
>> drivers/phy/qualcomm/phy-qcom-qmp.c | 13 +++++
>> 5 files changed, 36 insertions(+), 117 deletions(-)
>>
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/2] eDP/DP Phy vdda realted function
2022-05-18 17:29 ` Kuogee Hsieh
@ 2022-05-18 17:31 ` Dmitry Baryshkov
2022-05-18 20:19 ` Kuogee Hsieh
0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Baryshkov @ 2022-05-18 17:31 UTC (permalink / raw)
To: Kuogee Hsieh
Cc: dri-devel, robdclark, sean, swboyd, dianders, vkoul, daniel,
airlied, agross, bjorn.andersson, quic_abhinavk, quic_aravindh,
quic_sbillaka, freedreno, linux-arm-msm, linux-kernel
On 18/05/2022 20:29, Kuogee Hsieh wrote:
>
> On 5/18/2022 10:16 AM, Dmitry Baryshkov wrote:
>> On Wed, 18 May 2022 at 19:43, Kuogee Hsieh <quic_khsieh@quicinc.com>
>> wrote:
>>> 1) add regulator_set_load() to eDP/DP phy
>>> 2) remove vdda related function out of eDP/DP controller
>> These patches touch two subsystems and have a dependency between them.
>> How do we merge them?
>
> currently, both phy and controller are vote for regulator. The last vote
> will just increase count.
>
> Therefore the dependency should be very loose.
So, do you propose to merge dp change a cycle after the phy changes go in?
>
>
>>> Kuogee Hsieh (2):
>>> phy/qcom: add regulator_set_load to edp/dp phy
>>> drm/msm/dp: delete vdda regulator related functions from eDP/DP
>>> controller
>>>
>>> drivers/gpu/drm/msm/dp/dp_parser.c | 14 ------
>>> drivers/gpu/drm/msm/dp/dp_parser.h | 6 ---
>>> drivers/gpu/drm/msm/dp/dp_power.c | 95
>>> +------------------------------------
>>> drivers/phy/qualcomm/phy-qcom-edp.c | 25 ++++++++--
>>> drivers/phy/qualcomm/phy-qcom-qmp.c | 13 +++++
>>> 5 files changed, 36 insertions(+), 117 deletions(-)
>>>
>>> --
>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>> Forum,
>>> a Linux Foundation Collaborative Project
>>>
>>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 0/2] eDP/DP Phy vdda realted function
2022-05-18 17:31 ` Dmitry Baryshkov
@ 2022-05-18 20:19 ` Kuogee Hsieh
0 siblings, 0 replies; 12+ messages in thread
From: Kuogee Hsieh @ 2022-05-18 20:19 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: dri-devel, robdclark, sean, swboyd, dianders, vkoul, daniel,
airlied, agross, bjorn.andersson, quic_abhinavk, quic_aravindh,
quic_sbillaka, freedreno, linux-arm-msm, linux-kernel
On 5/18/2022 10:31 AM, Dmitry Baryshkov wrote:
> On 18/05/2022 20:29, Kuogee Hsieh wrote:
>>
>> On 5/18/2022 10:16 AM, Dmitry Baryshkov wrote:
>>> On Wed, 18 May 2022 at 19:43, Kuogee Hsieh <quic_khsieh@quicinc.com>
>>> wrote:
>>>> 1) add regulator_set_load() to eDP/DP phy
>>>> 2) remove vdda related function out of eDP/DP controller
>>> These patches touch two subsystems and have a dependency between them.
>>> How do we merge them?
>>
>> currently, both phy and controller are vote for regulator. The last
>> vote will just increase count.
>>
>> Therefore the dependency should be very loose.
>
> So, do you propose to merge dp change a cycle after the phy changes go
> in?
>
yes,
>>
>>
>>>> Kuogee Hsieh (2):
>>>> phy/qcom: add regulator_set_load to edp/dp phy
>>>> drm/msm/dp: delete vdda regulator related functions from eDP/DP
>>>> controller
>>>>
>>>> drivers/gpu/drm/msm/dp/dp_parser.c | 14 ------
>>>> drivers/gpu/drm/msm/dp/dp_parser.h | 6 ---
>>>> drivers/gpu/drm/msm/dp/dp_power.c | 95
>>>> +------------------------------------
>>>> drivers/phy/qualcomm/phy-qcom-edp.c | 25 ++++++++--
>>>> drivers/phy/qualcomm/phy-qcom-qmp.c | 13 +++++
>>>> 5 files changed, 36 insertions(+), 117 deletions(-)
>>>>
>>>> --
>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
>>>> Forum,
>>>> a Linux Foundation Collaborative Project
>>>>
>>>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread