[PATCH 0/3] pci: qcom: Add PCIe setting current load support

[PATCH 0/3] pci: qcom: Add PCIe setting current load support

[Ziyue Zhang]

Base DT:
https://lore.kernel.org/all/20241122023314.1616353-1-quic_ziyuzhan@quicinc.com/

This series add PCIe current load vote/devote for PCIe PHY driver, add
vdda-pll-max-microamp property in DT, and also document current load
properties in dt-bindings.
 
On platform QCS615, the current that phy consumes will exceed the maximum
current the regulator can provide in LPM mode, leading to over current
protection and system boot up stuck. 

This series can fix the issue by setting current load to an expected value
parsed from DT. This will vote the regulator to work in HPM mode so that
it is able to output a larger current and viod over current protection.
When the PCIe PHY poweroff in case like system suspend or shutdown, it
will also devote regulator back to LPM mode to decline regulator itself's
power consumption by setting load to zero.

Besides, three optional current load properties are added in dt-bindings, 
vdda-phy-max-microamp, vdda-pll-max-microamp and vdda-qref-max-microamp.
PCIe PHY that wants to vote for more current consumption should provide
corresponding property.

Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
---
Have following changes:
	- Add optional current load properties
	- Add pcie phy max current property.
	- Add current load vote/devote for PCIe PHY

Ziyue Zhang (3):
  dt-bindings: phy: qcom,qmp-pcie: add optional current load properties
  phy: qcom: qmp-pcie: add current load vote/devote for PCIe PHY
  arm64: dts: qcom: qcs615: add pcie phy max current property

 .../phy/qcom,sc8280xp-qmp-pcie-phy.yaml       |  8 +++++
 arch/arm64/boot/dts/qcom/qcs615-ride.dts      |  1 +
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c      | 35 +++++++++++++++++--
 3 files changed, 42 insertions(+), 2 deletions(-)


base-commit: ced7ce570dca175d87392ebaacf6c75f93aa2418
-- 
2.34.1


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

[PATCH 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current load properties

[Ziyue Zhang]

On some platforms, the power supply for PCIe PHY is not able to provide
enough current when it works in LPM mode. Hence, PCIe PHY driver needs to
set current load to vote the regulator to HPM mode.

Document the current load as properties for each power supply PCIe PHY
required, namely vdda-phy-max-microamp, vdda-pll-max-microamp and
vdda-qref-max-microamp, respectively.PCIe PHY driver should parse them to
set appropriate current load during PHY power on.

This three properties are optional and not mandatory for those platforms
that PCIe PHY can still work with power supply.

Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
---
 .../bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml          | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml
index 34d977af9263..0e2715301c54 100644
--- a/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,sc8280xp-qmp-pcie-phy.yaml
@@ -78,10 +78,16 @@ properties:
 
   vdda-phy-supply: true
 
+  vdda-phy-max-microamp: true
+
   vdda-pll-supply: true
 
+  vdda-pll-max-microamp: true
+
   vdda-qref-supply: true
 
+  vdda-qref-max-microamp: true
+
   qcom,4ln-config-sel:
     description: PCIe 4-lane configuration
     $ref: /schemas/types.yaml#/definitions/phandle-array
@@ -261,6 +267,7 @@ examples:
 
       vdda-phy-supply = <&vreg_l6d>;
       vdda-pll-supply = <&vreg_l4d>;
+      vdda-pll-max-microamp = <165000>;
 
       #clock-cells = <0>;
       clock-output-names = "pcie_2b_pipe_clk";
@@ -288,6 +295,7 @@ examples:
 
       vdda-phy-supply = <&vreg_l6d>;
       vdda-pll-supply = <&vreg_l4d>;
+      vdda-pll-max-microamp = <165000>;
 
       qcom,4ln-config-sel = <&tcsr 0xa044 0>;
 
-- 
2.34.1


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

[PATCH 2/3] phy: qcom: qmp-pcie: add current load vote/devote for PCIe PHY

[Ziyue Zhang]

On some platform (eg.qcs615), the current that phy consumes will exceed
the maximum current the regulator can provide in LPM mode, leading to
over current protection and system boot up stuck. Fix this issue by
setting regulator load to an expected value getting from phy device tree
node during init so that the regulator can scale up to HPM to allow a
larger current load.
This change will also set load to zero during deinit to let regulator
scale down to LPM mode to reduce itself's power consumptionif PCIe
suspend.

Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 35 ++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index c8e39c147ba4..782d51ab5cf1 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -39,6 +39,7 @@
 #include "phy-qcom-qmp-pcie-qhp.h"
 
 #define PHY_INIT_COMPLETE_TIMEOUT		10000
+#define MAX_PROP_SIZE		   32
 
 /* set of registers with offsets different per-PHY */
 enum qphy_reg_layout {
@@ -2905,6 +2906,7 @@ struct qmp_pcie {
 	struct reset_control_bulk_data *resets;
 	struct reset_control *nocsr_reset;
 	struct regulator_bulk_data *vregs;
+	u32 *max_current_load;
 
 	struct phy *phy;
 	int mode;
@@ -4087,6 +4089,17 @@ static int qmp_pcie_init(struct phy *phy)
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 	int ret;
 
+	for (int i = 0; i < cfg->num_vregs; i++) {
+		if (qmp->max_current_load[i]) {
+			ret = regulator_set_load(qmp->vregs[i].consumer, qmp->max_current_load[i]);
+			if (ret) {
+				dev_err(&phy->dev,
+					"failed to set load at %s\n", qmp->vregs[i].supply);
+				return ret;
+			}
+		}
+	}
+
 	ret = regulator_bulk_enable(cfg->num_vregs, qmp->vregs);
 	if (ret) {
 		dev_err(qmp->dev, "failed to enable regulators, err=%d\n", ret);
@@ -4129,6 +4142,7 @@ static int qmp_pcie_init(struct phy *phy)
 
 static int qmp_pcie_exit(struct phy *phy)
 {
+	int ret;
 	struct qmp_pcie *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 
@@ -4137,7 +4151,16 @@ static int qmp_pcie_exit(struct phy *phy)
 	clk_bulk_disable_unprepare(ARRAY_SIZE(qmp_pciephy_clk_l), qmp->clks);
 
 	regulator_bulk_disable(cfg->num_vregs, qmp->vregs);
-
+	for (int i = 0; i < cfg->num_vregs; i++) {
+		if (qmp->max_current_load[i]) {
+			ret = regulator_set_load(qmp->vregs[i].consumer, 0);
+			if (ret) {
+				dev_err(&phy->dev,
+					"failed to set load at %s\n", qmp->vregs[i].supply);
+				return ret;
+			}
+		}
+	}
 	return 0;
 }
 
@@ -4274,14 +4297,22 @@ static int qmp_pcie_vreg_init(struct qmp_pcie *qmp)
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 	struct device *dev = qmp->dev;
 	int num = cfg->num_vregs;
+	char prop_name[MAX_PROP_SIZE];
 	int i;
 
 	qmp->vregs = devm_kcalloc(dev, num, sizeof(*qmp->vregs), GFP_KERNEL);
 	if (!qmp->vregs)
 		return -ENOMEM;
 
-	for (i = 0; i < num; i++)
+	qmp->max_current_load = devm_kcalloc(dev, num, sizeof(*qmp->max_current_load), GFP_KERNEL);
+	if (!qmp->max_current_load)
+		return -ENOMEM;
+
+	for (i = 0; i < num; i++) {
 		qmp->vregs[i].supply = cfg->vreg_list[i];
+		snprintf(prop_name, MAX_PROP_SIZE, "%s-max-microamp", qmp->vregs[i].supply);
+		of_property_read_u32(qmp->dev->of_node, prop_name, &qmp->max_current_load[i]);
+	}
 
 	return devm_regulator_bulk_get(dev, num, qmp->vregs);
 }
-- 
2.34.1


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

[PATCH 3/3] arm64: dts: qcom: qcs615: add pcie phy max current property

[Ziyue Zhang]

Add vdda-pll-max-microamp for vdda-pll-supply. The value of this property
is from the power grid guide. It is the maximum current the regulator can
provide. The property will be parsed by PCIe PHY driver to set the current
load.

Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
---
 arch/arm64/boot/dts/qcom/qcs615-ride.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
index 18f131ae9e07..6d93ef0d886b 100644
--- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
+++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
@@ -215,6 +215,7 @@ &pcie {
 &pcie_phy {
 	vdda-phy-supply = <&vreg_l5a>;
 	vdda-pll-supply = <&vreg_l12a>;
+	vdda-pll-max-microamp = <165000>;
 
 	status = "okay";
 };
-- 
2.34.1


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

Re: [PATCH 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current load properties

[Krzysztof Kozlowski]

On Wed, Dec 04, 2024 at 06:52:47PM +0800, Ziyue Zhang wrote:
> On some platforms, the power supply for PCIe PHY is not able to provide
> enough current when it works in LPM mode. Hence, PCIe PHY driver needs to
> set current load to vote the regulator to HPM mode.
> 
> Document the current load as properties for each power supply PCIe PHY
> required, namely vdda-phy-max-microamp, vdda-pll-max-microamp and
> vdda-qref-max-microamp, respectively.PCIe PHY driver should parse them to
> set appropriate current load during PHY power on.
> 
> This three properties are optional and not mandatory for those platforms
> that PCIe PHY can still work with power supply.


Uh uh, so the downstream comes finally!

No sorry guys, use existing regulator bindings for this.

Best regards,
Krzysztof


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

Re: [PATCH 2/3] phy: qcom: qmp-pcie: add current load vote/devote for PCIe PHY

[Konrad Dybcio]

On 4.12.2024 11:52 AM, Ziyue Zhang wrote:
> On some platform (eg.qcs615), the current that phy consumes will exceed
> the maximum current the regulator can provide in LPM mode, leading to
> over current protection and system boot up stuck. Fix this issue by
> setting regulator load to an expected value getting from phy device tree
> node during init so that the regulator can scale up to HPM to allow a
> larger current load.
> This change will also set load to zero during deinit to let regulator
> scale down to LPM mode to reduce itself's power consumptionif PCIe
> suspend.
> 
> Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 35 ++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index c8e39c147ba4..782d51ab5cf1 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -39,6 +39,7 @@
>  #include "phy-qcom-qmp-pcie-qhp.h"
>  
>  #define PHY_INIT_COMPLETE_TIMEOUT		10000
> +#define MAX_PROP_SIZE		   32
>  
>  /* set of registers with offsets different per-PHY */
>  enum qphy_reg_layout {
> @@ -2905,6 +2906,7 @@ struct qmp_pcie {
>  	struct reset_control_bulk_data *resets;
>  	struct reset_control *nocsr_reset;
>  	struct regulator_bulk_data *vregs;
> +	u32 *max_current_load;
>  
>  	struct phy *phy;
>  	int mode;
> @@ -4087,6 +4089,17 @@ static int qmp_pcie_init(struct phy *phy)
>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
>  	int ret;
>  
> +	for (int i = 0; i < cfg->num_vregs; i++) {
> +		if (qmp->max_current_load[i]) {
> +			ret = regulator_set_load(qmp->vregs[i].consumer, qmp->max_current_load[i]);

I think it's better if we just put this info in the driver, like with
e.g. the DSI PHY

Konrad

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

Re: [PATCH 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current load properties

[Manivannan Sadhasivam]

On Thu, Dec 05, 2024 at 11:23:11AM +0100, Krzysztof Kozlowski wrote:
> On Wed, Dec 04, 2024 at 06:52:47PM +0800, Ziyue Zhang wrote:
> > On some platforms, the power supply for PCIe PHY is not able to provide
> > enough current when it works in LPM mode. Hence, PCIe PHY driver needs to
> > set current load to vote the regulator to HPM mode.
> > 
> > Document the current load as properties for each power supply PCIe PHY
> > required, namely vdda-phy-max-microamp, vdda-pll-max-microamp and
> > vdda-qref-max-microamp, respectively.PCIe PHY driver should parse them to
> > set appropriate current load during PHY power on.
> > 
> > This three properties are optional and not mandatory for those platforms
> > that PCIe PHY can still work with power supply.
> 
> 
> Uh uh, so the downstream comes finally!
> 
> No sorry guys, use existing regulator bindings for this.
> 

Maybe they got inspired by upstream UFS bindings?
Documentation/devicetree/bindings/ufs/ufs-common.yaml:

vcc-max-microamp
vccq-max-microamp
vccq2-max-microamp

Regulator binding only describes the min/max load for the regulators and not
consumers. What if the consumers need to set variable load per platform? Should
they hardcode the load in driver? (even so, the load should not vary for each
board).

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

Re: [PATCH 3/3] arm64: dts: qcom: qcs615: add pcie phy max current property

[Manivannan Sadhasivam]

On Wed, Dec 04, 2024 at 06:52:49PM +0800, Ziyue Zhang wrote:
> Add vdda-pll-max-microamp for vdda-pll-supply. The value of this property
> is from the power grid guide. It is the maximum current the regulator can
> provide. The property will be parsed by PCIe PHY driver to set the current
> load.
> 
> Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs615-ride.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> index 18f131ae9e07..6d93ef0d886b 100644
> --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> @@ -215,6 +215,7 @@ &pcie {
>  &pcie_phy {
>  	vdda-phy-supply = <&vreg_l5a>;
>  	vdda-pll-supply = <&vreg_l12a>;
> +	vdda-pll-max-microamp = <165000>;
>  

Min uV of this regulator is 1800000:
https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/tree/arch/arm64/boot/dts/qcom/qcs615-ride.dts?h=for-next#n151

How can you set 165000?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

Re: [PATCH 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current load properties

[Krzysztof Kozlowski]

On 11/12/2024 07:20, Manivannan Sadhasivam wrote:
> On Thu, Dec 05, 2024 at 11:23:11AM +0100, Krzysztof Kozlowski wrote:
>> On Wed, Dec 04, 2024 at 06:52:47PM +0800, Ziyue Zhang wrote:
>>> On some platforms, the power supply for PCIe PHY is not able to provide
>>> enough current when it works in LPM mode. Hence, PCIe PHY driver needs to
>>> set current load to vote the regulator to HPM mode.
>>>
>>> Document the current load as properties for each power supply PCIe PHY
>>> required, namely vdda-phy-max-microamp, vdda-pll-max-microamp and
>>> vdda-qref-max-microamp, respectively.PCIe PHY driver should parse them to
>>> set appropriate current load during PHY power on.
>>>
>>> This three properties are optional and not mandatory for those platforms
>>> that PCIe PHY can still work with power supply.
>>
>>
>> Uh uh, so the downstream comes finally!
>>
>> No sorry guys, use existing regulator bindings for this.
>>
> 
> Maybe they got inspired by upstream UFS bindings?
> Documentation/devicetree/bindings/ufs/ufs-common.yaml:
> 
> vcc-max-microamp
> vccq-max-microamp
> vccq2-max-microamp

And it is already an ABI, so we cannot do anything about it.

> 
> Regulator binding only describes the min/max load for the regulators and not

No, it exactly describes min/max consumers can use. Let's quote:
"largest current consumers may set"
It is all about consumers.

> consumers. What if the consumers need to set variable load per platform? Should

Then each platform uses regulator API or regulator bindings to set it? I
don't see the problem here.

> they hardcode the load in driver? (even so, the load should not vary for each
> board).

The load must vary per board, because regulators vary per board. Of
course in practice most designs could be the same, but regulators and
their limits are always properties of the board, not the SoC.

Best regards,
Krzysztof

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

Re: [PATCH 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current load properties

[Manivannan Sadhasivam]

On Wed, Dec 11, 2024 at 09:09:18AM +0100, Krzysztof Kozlowski wrote:
> On 11/12/2024 07:20, Manivannan Sadhasivam wrote:
> > On Thu, Dec 05, 2024 at 11:23:11AM +0100, Krzysztof Kozlowski wrote:
> >> On Wed, Dec 04, 2024 at 06:52:47PM +0800, Ziyue Zhang wrote:
> >>> On some platforms, the power supply for PCIe PHY is not able to provide
> >>> enough current when it works in LPM mode. Hence, PCIe PHY driver needs to
> >>> set current load to vote the regulator to HPM mode.
> >>>
> >>> Document the current load as properties for each power supply PCIe PHY
> >>> required, namely vdda-phy-max-microamp, vdda-pll-max-microamp and
> >>> vdda-qref-max-microamp, respectively.PCIe PHY driver should parse them to
> >>> set appropriate current load during PHY power on.
> >>>
> >>> This three properties are optional and not mandatory for those platforms
> >>> that PCIe PHY can still work with power supply.
> >>
> >>
> >> Uh uh, so the downstream comes finally!
> >>
> >> No sorry guys, use existing regulator bindings for this.
> >>
> > 
> > Maybe they got inspired by upstream UFS bindings?
> > Documentation/devicetree/bindings/ufs/ufs-common.yaml:
> > 
> > vcc-max-microamp
> > vccq-max-microamp
> > vccq2-max-microamp
> 
> And it is already an ABI, so we cannot do anything about it.
> 
> > 
> > Regulator binding only describes the min/max load for the regulators and not
> 
> No, it exactly describes min/max consumers can use. Let's quote:
> "largest current consumers may set"
> It is all about consumers.
> 
> > consumers. What if the consumers need to set variable load per platform? Should
> 
> Then each platform uses regulator API or regulator bindings to set it? I
> don't see the problem here.
> 
> > they hardcode the load in driver? (even so, the load should not vary for each
> > board).
> 
> The load must vary per board, because regulators vary per board. Of
> course in practice most designs could be the same, but regulators and
> their limits are always properties of the board, not the SoC.
> 

How the consumer drivers are supposed to know the optimum load?

I don't see how the consumer drivers can set the load without hardcoding the
values. And I could see from UFS properties that each board has different
values.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

Re: [PATCH 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current load properties

[Krzysztof Kozlowski]

On 11/12/2024 09:24, Manivannan Sadhasivam wrote:
> On Wed, Dec 11, 2024 at 09:09:18AM +0100, Krzysztof Kozlowski wrote:
>> On 11/12/2024 07:20, Manivannan Sadhasivam wrote:
>>> On Thu, Dec 05, 2024 at 11:23:11AM +0100, Krzysztof Kozlowski wrote:
>>>> On Wed, Dec 04, 2024 at 06:52:47PM +0800, Ziyue Zhang wrote:
>>>>> On some platforms, the power supply for PCIe PHY is not able to provide
>>>>> enough current when it works in LPM mode. Hence, PCIe PHY driver needs to
>>>>> set current load to vote the regulator to HPM mode.
>>>>>
>>>>> Document the current load as properties for each power supply PCIe PHY
>>>>> required, namely vdda-phy-max-microamp, vdda-pll-max-microamp and
>>>>> vdda-qref-max-microamp, respectively.PCIe PHY driver should parse them to
>>>>> set appropriate current load during PHY power on.
>>>>>
>>>>> This three properties are optional and not mandatory for those platforms
>>>>> that PCIe PHY can still work with power supply.
>>>>
>>>>
>>>> Uh uh, so the downstream comes finally!
>>>>
>>>> No sorry guys, use existing regulator bindings for this.
>>>>
>>>
>>> Maybe they got inspired by upstream UFS bindings?
>>> Documentation/devicetree/bindings/ufs/ufs-common.yaml:
>>>
>>> vcc-max-microamp
>>> vccq-max-microamp
>>> vccq2-max-microamp
>>
>> And it is already an ABI, so we cannot do anything about it.
>>
>>>
>>> Regulator binding only describes the min/max load for the regulators and not
>>
>> No, it exactly describes min/max consumers can use. Let's quote:
>> "largest current consumers may set"
>> It is all about consumers.
>>
>>> consumers. What if the consumers need to set variable load per platform? Should
>>
>> Then each platform uses regulator API or regulator bindings to set it? I
>> don't see the problem here.
>>
>>> they hardcode the load in driver? (even so, the load should not vary for each
>>> board).
>>
>> The load must vary per board, because regulators vary per board. Of
>> course in practice most designs could be the same, but regulators and
>> their limits are always properties of the board, not the SoC.
>>
> 
> How the consumer drivers are supposed to know the optimum load?
> 
> I don't see how the consumer drivers can set the load without hardcoding the
> values. And I could see from UFS properties that each board has different
> values.


Drivers do not need to know, it's not the driver's responsibility. If
these are constraints per board, then regulator properties apply and
there is no difference between this "vdd-max-microamp = 10" and
"regulator-max-microamp".

If this varies runtime, then your property is already not suitable and
very limited and you should use OPP table.

Best regards,
Krzysztof

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

Re: [PATCH 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current load properties

[Krzysztof Kozlowski]

On 11/12/2024 10:52, Krzysztof Kozlowski wrote:
> On 11/12/2024 09:24, Manivannan Sadhasivam wrote:
>> On Wed, Dec 11, 2024 at 09:09:18AM +0100, Krzysztof Kozlowski wrote:
>>> On 11/12/2024 07:20, Manivannan Sadhasivam wrote:
>>>> On Thu, Dec 05, 2024 at 11:23:11AM +0100, Krzysztof Kozlowski wrote:
>>>>> On Wed, Dec 04, 2024 at 06:52:47PM +0800, Ziyue Zhang wrote:
>>>>>> On some platforms, the power supply for PCIe PHY is not able to provide
>>>>>> enough current when it works in LPM mode. Hence, PCIe PHY driver needs to
>>>>>> set current load to vote the regulator to HPM mode.
>>>>>>
>>>>>> Document the current load as properties for each power supply PCIe PHY
>>>>>> required, namely vdda-phy-max-microamp, vdda-pll-max-microamp and
>>>>>> vdda-qref-max-microamp, respectively.PCIe PHY driver should parse them to
>>>>>> set appropriate current load during PHY power on.
>>>>>>
>>>>>> This three properties are optional and not mandatory for those platforms
>>>>>> that PCIe PHY can still work with power supply.
>>>>>
>>>>>
>>>>> Uh uh, so the downstream comes finally!
>>>>>
>>>>> No sorry guys, use existing regulator bindings for this.
>>>>>
>>>>
>>>> Maybe they got inspired by upstream UFS bindings?
>>>> Documentation/devicetree/bindings/ufs/ufs-common.yaml:
>>>>
>>>> vcc-max-microamp
>>>> vccq-max-microamp
>>>> vccq2-max-microamp
>>>
>>> And it is already an ABI, so we cannot do anything about it.
>>>
>>>>
>>>> Regulator binding only describes the min/max load for the regulators and not
>>>
>>> No, it exactly describes min/max consumers can use. Let's quote:
>>> "largest current consumers may set"
>>> It is all about consumers.
>>>
>>>> consumers. What if the consumers need to set variable load per platform? Should
>>>
>>> Then each platform uses regulator API or regulator bindings to set it? I
>>> don't see the problem here.
>>>
>>>> they hardcode the load in driver? (even so, the load should not vary for each
>>>> board).
>>>
>>> The load must vary per board, because regulators vary per board. Of
>>> course in practice most designs could be the same, but regulators and
>>> their limits are always properties of the board, not the SoC.
>>>
>>
>> How the consumer drivers are supposed to know the optimum load?
>>
>> I don't see how the consumer drivers can set the load without hardcoding the
>> values. And I could see from UFS properties that each board has different
>> values.
> 
> 
> Drivers do not need to know, it's not the driver's responsibility. If
> these are constraints per board, then regulator properties apply and
> there is no difference between this "vdd-max-microamp = 10" and
> "regulator-max-microamp".
> 
> If this varies runtime, then your property is already not suitable and
> very limited and you should use OPP table.
> 
Plus let's recap the commit msg which is supposed to fully explain the
hardware:
"the power supply for PCIe PHY is not able to provide
enough current"

Well, that's 100% property of power supply - so the regulator node in
PMIC, not the UFS device (consumer).

With that commit msg it is clear the properties are placed wrongly.
However maybe just commit msg is wrong, but then how could we possibly
know what is the real hardware, right? :)


Best regards,
Krzysztof

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

Re: [PATCH 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current load properties

[Manivannan Sadhasivam]

On Wed, Dec 11, 2024 at 10:52:11AM +0100, Krzysztof Kozlowski wrote:
> On 11/12/2024 09:24, Manivannan Sadhasivam wrote:
> > On Wed, Dec 11, 2024 at 09:09:18AM +0100, Krzysztof Kozlowski wrote:
> >> On 11/12/2024 07:20, Manivannan Sadhasivam wrote:
> >>> On Thu, Dec 05, 2024 at 11:23:11AM +0100, Krzysztof Kozlowski wrote:
> >>>> On Wed, Dec 04, 2024 at 06:52:47PM +0800, Ziyue Zhang wrote:
> >>>>> On some platforms, the power supply for PCIe PHY is not able to provide
> >>>>> enough current when it works in LPM mode. Hence, PCIe PHY driver needs to
> >>>>> set current load to vote the regulator to HPM mode.
> >>>>>
> >>>>> Document the current load as properties for each power supply PCIe PHY
> >>>>> required, namely vdda-phy-max-microamp, vdda-pll-max-microamp and
> >>>>> vdda-qref-max-microamp, respectively.PCIe PHY driver should parse them to
> >>>>> set appropriate current load during PHY power on.
> >>>>>
> >>>>> This three properties are optional and not mandatory for those platforms
> >>>>> that PCIe PHY can still work with power supply.
> >>>>
> >>>>
> >>>> Uh uh, so the downstream comes finally!
> >>>>
> >>>> No sorry guys, use existing regulator bindings for this.
> >>>>
> >>>
> >>> Maybe they got inspired by upstream UFS bindings?
> >>> Documentation/devicetree/bindings/ufs/ufs-common.yaml:
> >>>
> >>> vcc-max-microamp
> >>> vccq-max-microamp
> >>> vccq2-max-microamp
> >>
> >> And it is already an ABI, so we cannot do anything about it.
> >>
> >>>
> >>> Regulator binding only describes the min/max load for the regulators and not
> >>
> >> No, it exactly describes min/max consumers can use. Let's quote:
> >> "largest current consumers may set"
> >> It is all about consumers.
> >>
> >>> consumers. What if the consumers need to set variable load per platform? Should
> >>
> >> Then each platform uses regulator API or regulator bindings to set it? I
> >> don't see the problem here.
> >>
> >>> they hardcode the load in driver? (even so, the load should not vary for each
> >>> board).
> >>
> >> The load must vary per board, because regulators vary per board. Of
> >> course in practice most designs could be the same, but regulators and
> >> their limits are always properties of the board, not the SoC.
> >>
> > 
> > How the consumer drivers are supposed to know the optimum load?
> > 
> > I don't see how the consumer drivers can set the load without hardcoding the
> > values. And I could see from UFS properties that each board has different
> > values.
> 
> 
> Drivers do not need to know, it's not the driver's responsibility. If

What? I think there is a misunderstanding here. The intention of these proposed
properties is to allow the PHY driver to set the required load of the regulator
using regulator_set_load() API. As per the API description:

'Consumer devices notify their supply regulator of the maximum power they will
require (can be taken from device datasheet in the power consumption tables)
when they change operational status and hence power state'.

IIUC, your concern is that the devicetree shouldn't specify the load for each
consumer but just the min/max load of the regulator. And the consumer driver
should figure out the load and set it accordingly.

Correct me if I'm wrong.

In that case, I was wondering if the load set by the driver is going to vary
between platforms (boards) or not (question to Ziyue Zhang). If it varies
between SoC, then we can hardcode the load in driver based on compatible.

> If
> these are constraints per board, then regulator properties apply and
> there is no difference between this "vdd-max-microamp = 10" and
> "regulator-max-microamp".
> 

There is a difference. Regulator properties are just threshold. So unless the
consumer sets the load, they won't take effect. I think you got confused by the
'max' wording in the proposed properties?

> If this varies runtime, then your property is already not suitable and
> very limited and you should use OPP table.
> 

The consumer driver may request different loads based on their operational
state.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

Re: [PATCH 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current load properties

[Ziyue Zhang]

在 12/11/2024 7:50 PM, Manivannan Sadhasivam 写道:
> On Wed, Dec 11, 2024 at 10:52:11AM +0100, Krzysztof Kozlowski wrote:
>> On 11/12/2024 09:24, Manivannan Sadhasivam wrote:
>>> On Wed, Dec 11, 2024 at 09:09:18AM +0100, Krzysztof Kozlowski wrote:
>>>> On 11/12/2024 07:20, Manivannan Sadhasivam wrote:
>>>>> On Thu, Dec 05, 2024 at 11:23:11AM +0100, Krzysztof Kozlowski wrote:
>>>>>> On Wed, Dec 04, 2024 at 06:52:47PM +0800, Ziyue Zhang wrote:
>>>>>>> On some platforms, the power supply for PCIe PHY is not able to provide
>>>>>>> enough current when it works in LPM mode. Hence, PCIe PHY driver needs to
>>>>>>> set current load to vote the regulator to HPM mode.
>>>>>>>
>>>>>>> Document the current load as properties for each power supply PCIe PHY
>>>>>>> required, namely vdda-phy-max-microamp, vdda-pll-max-microamp and
>>>>>>> vdda-qref-max-microamp, respectively.PCIe PHY driver should parse them to
>>>>>>> set appropriate current load during PHY power on.
>>>>>>>
>>>>>>> This three properties are optional and not mandatory for those platforms
>>>>>>> that PCIe PHY can still work with power supply.
>>>>>>
>>>>>> Uh uh, so the downstream comes finally!
>>>>>>
>>>>>> No sorry guys, use existing regulator bindings for this.
>>>>>>
>>>>> Maybe they got inspired by upstream UFS bindings?
>>>>> Documentation/devicetree/bindings/ufs/ufs-common.yaml:
>>>>>
>>>>> vcc-max-microamp
>>>>> vccq-max-microamp
>>>>> vccq2-max-microamp
>>>> And it is already an ABI, so we cannot do anything about it.
>>>>
>>>>> Regulator binding only describes the min/max load for the regulators and not
>>>> No, it exactly describes min/max consumers can use. Let's quote:
>>>> "largest current consumers may set"
>>>> It is all about consumers.
>>>>
>>>>> consumers. What if the consumers need to set variable load per platform? Should
>>>> Then each platform uses regulator API or regulator bindings to set it? I
>>>> don't see the problem here.
>>>>
>>>>> they hardcode the load in driver? (even so, the load should not vary for each
>>>>> board).
>>>> The load must vary per board, because regulators vary per board. Of
>>>> course in practice most designs could be the same, but regulators and
>>>> their limits are always properties of the board, not the SoC.
>>>>
>>> How the consumer drivers are supposed to know the optimum load?
>>>
>>> I don't see how the consumer drivers can set the load without hardcoding the
>>> values. And I could see from UFS properties that each board has different
>>> values.
>>
>> Drivers do not need to know, it's not the driver's responsibility. If
> What? I think there is a misunderstanding here. The intention of these proposed
> properties is to allow the PHY driver to set the required load of the regulator
> using regulator_set_load() API. As per the API description:
>
> 'Consumer devices notify their supply regulator of the maximum power they will
> require (can be taken from device datasheet in the power consumption tables)
> when they change operational status and hence power state'.
>
> IIUC, your concern is that the devicetree shouldn't specify the load for each
> consumer but just the min/max load of the regulator. And the consumer driver
> should figure out the load and set it accordingly.
>
> Correct me if I'm wrong.
>
> In that case, I was wondering if the load set by the driver is going to vary
> between platforms (boards) or not (question to Ziyue Zhang). If it varies
> between SoC, then we can hardcode the load in driver based on compatible.

Hi Mani, Krzystof

Now we set  the current to 165mA which is the max power supply the regulator
can provide, so this is platform(boards) related. But we think PCIe PHY needs
to set the current value we need, which is soc related.

BRs
Ziyue

>> If
>> these are constraints per board, then regulator properties apply and
>> there is no difference between this "vdd-max-microamp = 10" and
>> "regulator-max-microamp".
>>
> There is a difference. Regulator properties are just threshold. So unless the
> consumer sets the load, they won't take effect. I think you got confused by the
> 'max' wording in the proposed properties?
>
>> If this varies runtime, then your property is already not suitable and
>> very limited and you should use OPP table.
>>
> The consumer driver may request different loads based on their operational
> state.
>
> - Mani
>

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

Re: [PATCH 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current load properties

[Krzysztof Kozlowski]

On 11/12/2024 12:50, Manivannan Sadhasivam wrote:
> On Wed, Dec 11, 2024 at 10:52:11AM +0100, Krzysztof Kozlowski wrote:
>> On 11/12/2024 09:24, Manivannan Sadhasivam wrote:
>>> On Wed, Dec 11, 2024 at 09:09:18AM +0100, Krzysztof Kozlowski wrote:
>>>> On 11/12/2024 07:20, Manivannan Sadhasivam wrote:
>>>>> On Thu, Dec 05, 2024 at 11:23:11AM +0100, Krzysztof Kozlowski wrote:
>>>>>> On Wed, Dec 04, 2024 at 06:52:47PM +0800, Ziyue Zhang wrote:
>>>>>>> On some platforms, the power supply for PCIe PHY is not able to provide
>>>>>>> enough current when it works in LPM mode. Hence, PCIe PHY driver needs to
>>>>>>> set current load to vote the regulator to HPM mode.
>>>>>>>
>>>>>>> Document the current load as properties for each power supply PCIe PHY
>>>>>>> required, namely vdda-phy-max-microamp, vdda-pll-max-microamp and
>>>>>>> vdda-qref-max-microamp, respectively.PCIe PHY driver should parse them to
>>>>>>> set appropriate current load during PHY power on.
>>>>>>>
>>>>>>> This three properties are optional and not mandatory for those platforms
>>>>>>> that PCIe PHY can still work with power supply.
>>>>>>
>>>>>>
>>>>>> Uh uh, so the downstream comes finally!
>>>>>>
>>>>>> No sorry guys, use existing regulator bindings for this.
>>>>>>
>>>>>
>>>>> Maybe they got inspired by upstream UFS bindings?
>>>>> Documentation/devicetree/bindings/ufs/ufs-common.yaml:
>>>>>
>>>>> vcc-max-microamp
>>>>> vccq-max-microamp
>>>>> vccq2-max-microamp
>>>>
>>>> And it is already an ABI, so we cannot do anything about it.
>>>>
>>>>>
>>>>> Regulator binding only describes the min/max load for the regulators and not
>>>>
>>>> No, it exactly describes min/max consumers can use. Let's quote:
>>>> "largest current consumers may set"
>>>> It is all about consumers.
>>>>
>>>>> consumers. What if the consumers need to set variable load per platform? Should
>>>>
>>>> Then each platform uses regulator API or regulator bindings to set it? I
>>>> don't see the problem here.
>>>>
>>>>> they hardcode the load in driver? (even so, the load should not vary for each
>>>>> board).
>>>>
>>>> The load must vary per board, because regulators vary per board. Of
>>>> course in practice most designs could be the same, but regulators and
>>>> their limits are always properties of the board, not the SoC.
>>>>
>>>
>>> How the consumer drivers are supposed to know the optimum load?
>>>
>>> I don't see how the consumer drivers can set the load without hardcoding the
>>> values. And I could see from UFS properties that each board has different
>>> values.
>>
>>
>> Drivers do not need to know, it's not the driver's responsibility. If
> 
> What? I think there is a misunderstanding here. The intention of these proposed
> properties is to allow the PHY driver to set the required load of the regulator
> using regulator_set_load() API. As per the API description:

This makes sense. I referred to property description to learn what it is
really doing. I found nothing, because it is empty, so that's how we
waste time...

> 
> 'Consumer devices notify their supply regulator of the maximum power they will
> require (can be taken from device datasheet in the power consumption tables)
> when they change operational status and hence power state'.
> 
> IIUC, your concern is that the devicetree shouldn't specify the load for each

My concern is that Qualcomm put here regulator constraints and call them
device load. And empty property name does not help me to think other way.

How can I verify the number that it was taken from UFS device manual,
not from PMIC?

> consumer but just the min/max load of the regulator. And the consumer driver
> should figure out the load and set it accordingly.

With your above explanation I think the actual consumer load, if it is
depending on the board, have place in DT. But:

> 
> Correct me if I'm wrong.
> 
> In that case, I was wondering if the load set by the driver is going to vary
> between platforms (boards) or not (question to Ziyue Zhang). If it varies
> between SoC, then we can hardcode the load in driver based on compatible.

Yeah, maybe each  it is implied by compatible and not board dependent.
Then just like everything implied by compatibles: should not be placed
in DT.

We already follow this approach for several other Qualcomm drivers, e.g.
display.

> 
>> If
>> these are constraints per board, then regulator properties apply and
>> there is no difference between this "vdd-max-microamp = 10" and
>> "regulator-max-microamp".
>>
> 
> There is a difference. Regulator properties are just threshold. So unless the
> consumer sets the load, they won't take effect. I think you got confused by the
> 'max' wording in the proposed properties?

Yes and confused by its description in the binding.

> 
>> If this varies runtime, then your property is already not suitable and
>> very limited and you should use OPP table.
>>
> 
> The consumer driver may request different loads based on their operational
> state.

Then anyway maybe this should be OPP table (assuming it is board specific).

Best regards,
Krzysztof

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

Re: [PATCH 1/3] dt-bindings: phy: qcom,qmp-pcie: add optional current load properties

[Krzysztof Kozlowski]

On 12/12/2024 08:29, Ziyue Zhang wrote:
>
>> In that case, I was wondering if the load set by the driver is going to vary
>> between platforms (boards) or not (question to Ziyue Zhang). If it varies
>> between SoC, then we can hardcode the load in driver based on compatible.
> 
> Hi Mani, Krzystof
> 
> Now we set  the current to 165mA which is the max power supply the regulator
> can provide, so this is platform(boards) related. But we think PCIe PHY needs

Yeah, so that's the answer to what I asked just a second ago - you do
not put there device load. You put there regulator constraints.

> to set the current value we need, which is soc related.

So move it away from DT. I don't care what's in the driver, so you can
put there whatever fake value.

Best regards,
Krzysztof

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

Re: [PATCH 3/3] arm64: dts: qcom: qcs615: add pcie phy max current property

[Ziyue Zhang]

在 12/11/2024 2:26 PM, Manivannan Sadhasivam 写道:
> On Wed, Dec 04, 2024 at 06:52:49PM +0800, Ziyue Zhang wrote:
>> Add vdda-pll-max-microamp for vdda-pll-supply. The value of this property
>> is from the power grid guide. It is the maximum current the regulator can
>> provide. The property will be parsed by PCIe PHY driver to set the current
>> load.
>>
>> Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/qcs615-ride.dts | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>> index 18f131ae9e07..6d93ef0d886b 100644
>> --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>> +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
>> @@ -215,6 +215,7 @@ &pcie {
>>   &pcie_phy {
>>   	vdda-phy-supply = <&vreg_l5a>;
>>   	vdda-pll-supply = <&vreg_l12a>;
>> +	vdda-pll-max-microamp = <165000>;
>>   
> Min uV of this regulator is 1800000:
> https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git/tree/arch/arm64/boot/dts/qcom/qcs615-ride.dts?h=for-next#n151
>
> How can you set 165000?
>
> - Mani

Hi Mani
the 165000 cames from the power grid guide, and it is 165000uA not uV
BRs
Ziyue

>

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

Re: [PATCH 3/3] arm64: dts: qcom: qcs615: add pcie phy max current property

[Bjorn Andersson]

On Wed, Dec 04, 2024 at 06:52:49PM +0800, Ziyue Zhang wrote:
> Add vdda-pll-max-microamp for vdda-pll-supply. The value of this property
> is from the power grid guide. It is the maximum current the regulator can
> provide. The property will be parsed by PCIe PHY driver to set the current
> load.
> 
> Signed-off-by: Ziyue Zhang <quic_ziyuzhan@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/qcs615-ride.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/qcs615-ride.dts b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> index 18f131ae9e07..6d93ef0d886b 100644
> --- a/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs615-ride.dts
> @@ -215,6 +215,7 @@ &pcie {
>  &pcie_phy {
>  	vdda-phy-supply = <&vreg_l5a>;
>  	vdda-pll-supply = <&vreg_l12a>;
> +	vdda-pll-max-microamp = <165000>;

It seems from the driver patch (patch 2/3) that you will apply this
load-request at init and release it at exit, which I believe will hold
the regulator at HPM always.

If that's the case, why is vreg_l12a declared with
regulator-allow-set-load and support LPM on this board?

If the regulator needs to be in HPM on this board, remove
regulator-allow-set-load and the LPM mode from the regulator.


In fact, you (all of you) should remove all regulator-allow-set-load and
LPM modes from the DT until you know what that implies - and then
provide specific patches with clear description of the impact when you
add it back.

Regards,
Bjorn

>  
>  	status = "okay";
>  };
> -- 
> 2.34.1
> 

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