[PATCH] PCI: qcom: add missing supplies required for msm8996

[PATCH] PCI: qcom: add missing supplies required for msm8996

[srinivas.kandagatla]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

This patch adds supplies that are required for msm8996. Two of them vdda
and vdda-1p8 are analog supplies that go in to controller, and the rest
of the two vddpe's are supplies to PCIe endpoints.

Without these supplies PCIe endpoints which require power supplies are
not enumerated at all, as there is no one to power it up.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 .../devicetree/bindings/pci/qcom,pcie.txt          | 16 +++++++++++++
 drivers/pci/dwc/pcie-qcom.c                        | 28 ++++++++++++++++++++--
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
index 3c9d321b3d3b..045102cb3e12 100644
--- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
+++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
@@ -179,6 +179,11 @@
 	Value type: <phandle>
 	Definition: A phandle to the core analog power supply
 
+- vdda-1p8-supply:
+	Usage: required for msm8996
+	Value type: <phandle>
+	Definition: A phandle to the 1.8v analog power supply
+
 - vdda_phy-supply:
 	Usage: required for ipq/apq8064
 	Value type: <phandle>
@@ -189,6 +194,15 @@
 	Value type: <phandle>
 	Definition: A phandle to the analog power supply for IC which generates
 		    reference clock
+- vddpe-supply:
+	Usage: optional
+	Value type: <phandle>
+	Definition: A phandle to the PCIe endpoint power supply
+
+- vddpe1-supply:
+	Usage: optional
+	Value type: <phandle>
+	Definition: A phandle to the PCIe endpoint power supply 1
 
 - phys:
 	Usage: required for apq8084
@@ -205,6 +219,8 @@
 	Value type: <prop-encoded-array>
 	Definition: List of phandle and GPIO specifier pairs. Should contain
 			- "perst-gpios"	PCIe endpoint reset signal line
+			- "pe_en-gpios"	PCIe endpoint enable signal line
+			- "pe_en1-gpios" PCIe endpoint enable1 signal line
 			- "wake-gpios"	PCIe endpoint wake signal line
 
 * Example for ipq/apq8064
diff --git a/drivers/pci/dwc/pcie-qcom.c b/drivers/pci/dwc/pcie-qcom.c
index 952a4fc4bf3c..01488f90da31 100644
--- a/drivers/pci/dwc/pcie-qcom.c
+++ b/drivers/pci/dwc/pcie-qcom.c
@@ -109,13 +109,15 @@ struct qcom_pcie_resources_1_0_0 {
 	struct reset_control *core;
 	struct regulator *vdda;
 };
-
+#define QCOM_PCIE_MAX_SUPPLY	4
 struct qcom_pcie_resources_2_3_2 {
 	struct clk *aux_clk;
 	struct clk *master_clk;
 	struct clk *slave_clk;
 	struct clk *cfg_clk;
 	struct clk *pipe_clk;
+	int num_supplies;
+	struct regulator_bulk_data supplies[QCOM_PCIE_MAX_SUPPLY];
 };
 
 struct qcom_pcie_resources_2_4_0 {
@@ -529,6 +531,17 @@ static int qcom_pcie_get_resources_2_3_2(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
+	int ret;
+
+	res->supplies[0].supply = "vdda";
+	res->supplies[1].supply = "vdda-1p8";
+	res->supplies[2].supply = "vddpe";
+	res->supplies[3].supply = "vddpe1";
+	res->num_supplies = QCOM_PCIE_MAX_SUPPLY;
+	ret = devm_regulator_bulk_get(dev, QCOM_PCIE_MAX_SUPPLY,
+				      res->supplies);
+	if (ret)
+		return ret;
 
 	res->aux_clk = devm_clk_get(dev, "aux");
 	if (IS_ERR(res->aux_clk))
@@ -558,6 +571,8 @@ static void qcom_pcie_deinit_2_3_2(struct qcom_pcie *pcie)
 	clk_disable_unprepare(res->master_clk);
 	clk_disable_unprepare(res->cfg_clk);
 	clk_disable_unprepare(res->aux_clk);
+
+	regulator_bulk_disable(res->num_supplies, res->supplies);
 }
 
 static void qcom_pcie_post_deinit_2_3_2(struct qcom_pcie *pcie)
@@ -575,10 +590,16 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 	u32 val;
 	int ret;
 
+	ret = regulator_bulk_enable(res->num_supplies, res->supplies);
+	if (ret < 0) {
+		dev_err(dev, "cannot enable regulators\n");
+		return ret;
+	}
+
 	ret = clk_prepare_enable(res->aux_clk);
 	if (ret) {
 		dev_err(dev, "cannot prepare/enable aux clock\n");
-		return ret;
+		goto err_aux_clk;
 	}
 
 	ret = clk_prepare_enable(res->cfg_clk);
@@ -629,6 +650,9 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 err_cfg_clk:
 	clk_disable_unprepare(res->aux_clk);
 
+err_aux_clk:
+	regulator_bulk_disable(res->num_supplies, res->supplies);
+
 	return ret;
 }
 
-- 
2.15.0

Re: [PATCH] PCI: qcom: add missing supplies required for msm8996

[Rob Herring]

On Fri, Dec 08, 2017 at 09:20:53AM +0000, srinivas.kandagatla@linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> This patch adds supplies that are required for msm8996. Two of them vdda
> and vdda-1p8 are analog supplies that go in to controller, and the rest
> of the two vddpe's are supplies to PCIe endpoints.
> 
> Without these supplies PCIe endpoints which require power supplies are
> not enumerated at all, as there is no one to power it up.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt          | 16 +++++++++++++
>  drivers/pci/dwc/pcie-qcom.c                        | 28 ++++++++++++++++++++--
>  2 files changed, 42 insertions(+), 2 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>                  

Re: [PATCH] PCI: qcom: add missing supplies required for msm8996

[Stanimir Varbanov]

Hi Srini,

On 12/08/2017 11:20 AM, srinivas.kandagatla@linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> This patch adds supplies that are required for msm8996. Two of them vdda
> and vdda-1p8 are analog supplies that go in to controller, and the rest

According to the msm8996 device specification there are two pins related
to the PCIe power: VDD_PCIE_CORE (power for PCIe core circuitry) and
VDD_PCIE_1P8 (power for PCIe I/O circuitry). Thus I think it is clear
that VDD_PCIE_CORE is vdda and VDD_PCIE_1P8 should be part of PCIe phy
driver DT binding (and this is the case currently [1]). So I don't think
we need vdda-1p8 regulator DT binding for that in pcie-qcom.

> of the two vddpe's are supplies to PCIe endpoints.

For this part I'm still not sure. On first sight it looks that these
vdd's should be part of endpoint drivers, on the other hand we have
mPCIe connector (on db820c) which has two power rails 3p3v and 1p5v
which should be controlled/enabled as well.

So I'd like to hear more opinions on that, i.e. how this is solved by
the other PCIe bridge drivers.

> 
> Without these supplies PCIe endpoints which require power supplies are
> not enumerated at all, as there is no one to power it up.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt          | 16 +++++++++++++
>  drivers/pci/dwc/pcie-qcom.c                        | 28 ++++++++++++++++++++--
>  2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 3c9d321b3d3b..045102cb3e12 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -179,6 +179,11 @@
>  	Value type: <phandle>
>  	Definition: A phandle to the core analog power supply
>  
> +- vdda-1p8-supply:
> +	Usage: required for msm8996
> +	Value type: <phandle>
> +	Definition: A phandle to the 1.8v analog power supply
> +
>  - vdda_phy-supply:
>  	Usage: required for ipq/apq8064
>  	Value type: <phandle>
> @@ -189,6 +194,15 @@
>  	Value type: <phandle>
>  	Definition: A phandle to the analog power supply for IC which generates
>  		    reference clock
> +- vddpe-supply:
> +	Usage: optional
> +	Value type: <phandle>
> +	Definition: A phandle to the PCIe endpoint power supply
> +
> +- vddpe1-supply:
> +	Usage: optional
> +	Value type: <phandle>
> +	Definition: A phandle to the PCIe endpoint power supply 1
>  
>  - phys:
>  	Usage: required for apq8084
> @@ -205,6 +219,8 @@
>  	Value type: <prop-encoded-array>
>  	Definition: List of phandle and GPIO specifier pairs. Should contain
>  			- "perst-gpios"	PCIe endpoint reset signal line
> +			- "pe_en-gpios"	PCIe endpoint enable signal line
> +			- "pe_en1-gpios" PCIe endpoint enable1 signal line

those two shouldn't be here, instead they should be part of regulator DT
node, so please drop them.

>  			- "wake-gpios"	PCIe endpoint wake signal line
>  
>  * Example for ipq/apq8064
> diff --git a/drivers/pci/dwc/pcie-qcom.c b/drivers/pci/dwc/pcie-qcom.c
> index 952a4fc4bf3c..01488f90da31 100644
> --- a/drivers/pci/dwc/pcie-qcom.c
> +++ b/drivers/pci/dwc/pcie-qcom.c
> @@ -109,13 +109,15 @@ struct qcom_pcie_resources_1_0_0 {
>  	struct reset_control *core;
>  	struct regulator *vdda;
>  };
> -
> +#define QCOM_PCIE_MAX_SUPPLY	4
>  struct qcom_pcie_resources_2_3_2 {
>  	struct clk *aux_clk;
>  	struct clk *master_clk;
>  	struct clk *slave_clk;
>  	struct clk *cfg_clk;
>  	struct clk *pipe_clk;
> +	int num_supplies;
> +	struct regulator_bulk_data supplies[QCOM_PCIE_MAX_SUPPLY];
>  };
>  
>  struct qcom_pcie_resources_2_4_0 {
> @@ -529,6 +531,17 @@ static int qcom_pcie_get_resources_2_3_2(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> +	int ret;
> +
> +	res->supplies[0].supply = "vdda";
> +	res->supplies[1].supply = "vdda-1p8";
> +	res->supplies[2].supply = "vddpe";
> +	res->supplies[3].supply = "vddpe1";
> +	res->num_supplies = QCOM_PCIE_MAX_SUPPLY;
> +	ret = devm_regulator_bulk_get(dev, QCOM_PCIE_MAX_SUPPLY,
> +				      res->supplies);

If we decide to go on this direction we need to replace this with
devm_regulator_bulk_get_optional (yes I know there is no such regulator
API yet) because they are optional in the DT binding.

<snip>

-- 
regards,
Stan

[1]
https://elixir.free-electrons.com/linux/latest/source/arch/arm64/boot/dts/qcom/msm8996.dtsi#L638

Re: [PATCH] PCI: qcom: add missing supplies required for msm8996

[Srinivas Kandagatla]

On 14/12/17 10:06, Stanimir Varbanov wrote:
> Hi Srini,
> 
> On 12/08/2017 11:20 AM, srinivas.kandagatla@linaro.org wrote:
>> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>>
>> This patch adds supplies that are required for msm8996. Two of them vdda
>> and vdda-1p8 are analog supplies that go in to controller, and the rest
> 
> According to the msm8996 device specification there are two pins related
> to the PCIe power: VDD_PCIE_CORE (power for PCIe core circuitry) and
> VDD_PCIE_1P8 (power for PCIe I/O circuitry). Thus I think it is clear
> that VDD_PCIE_CORE is vdda and VDD_PCIE_1P8 should be part of PCIe phy
> driver DT binding (and this is the case currently [1]). So I don't think
> we need vdda-1p8 regulator DT binding for that in pcie-qcom.
Sure I will remove it.
> 
>> of the two vddpe's are supplies to PCIe endpoints.
> 
> For this part I'm still not sure. On first sight it looks that these
> vdd's should be part of endpoint drivers, on the other hand we have
> mPCIe connector (on db820c) which has two power rails 3p3v and 1p5v
> which should be controlled/enabled as well.
> 
Yes, these would be populated in DT as part of vddpe supplies for that lane.

> So I'd like to hear more opinions on that, i.e. how this is solved by
> the other PCIe bridge drivers.
I would be keen on knowing more options to solve this neatly too!!

This is only problem with DT and PCIE device discovery to start with!!

We are in catch 22 situation, where in we can not enumerate device 
without it actually powering up.

All the other controllers like SDIO, MMC use something similar to enable 
these regulators. SDIO actually uses pwrseq to do this.
There was attempt to generalize pwrseq to other subsystems, not sure 
where it landed.

As of today, the only way to power up endpoint in sync with reset is via 
controller driver, all other ways we would end up doing reset out of 
sync with power which are error prone.

> 

>>   - phys:
>>   	Usage: required for apq8084
>> @@ -205,6 +219,8 @@
>>   	Value type: <prop-encoded-array>
>>   	Definition: List of phandle and GPIO specifier pairs. Should contain
>>   			- "perst-gpios"	PCIe endpoint reset signal line
>> +			- "pe_en-gpios"	PCIe endpoint enable signal line
>> +			- "pe_en1-gpios" PCIe endpoint enable1 signal line
> 
> those two shouldn't be here, instead they should be part of regulator DT
> node, so please drop them.

Yes, I can drop them for now!

> 
>>   			- "wake-gpios"	PCIe endpoint wake signal line
>>   
>>   * Example for ipq/apq8064
>> diff --git a/drivers/pci/dwc/pcie-qcom.c b/drivers/pci/dwc/pcie-qcom.c
>> index 952a4fc4bf3c..01488f90da31 100644
>> --- a/drivers/pci/dwc/pcie-qcom.c
>> +++ b/drivers/pci/dwc/pcie-qcom.c
>> @@ -109,13 +109,15 @@ struct qcom_pcie_resources_1_0_0 {
>>   	struct reset_control *core;
>>   	struct regulator *vdda;
>>   };
>> -
>> +#define QCOM_PCIE_MAX_SUPPLY	4
>>   struct qcom_pcie_resources_2_3_2 {
>>   	struct clk *aux_clk;
>>   	struct clk *master_clk;
>>   	struct clk *slave_clk;
>>   	struct clk *cfg_clk;
>>   	struct clk *pipe_clk;
>> +	int num_supplies;
>> +	struct regulator_bulk_data supplies[QCOM_PCIE_MAX_SUPPLY];
>>   };
>>   
>>   struct qcom_pcie_resources_2_4_0 {
>> @@ -529,6 +531,17 @@ static int qcom_pcie_get_resources_2_3_2(struct qcom_pcie *pcie)
>>   	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
>>   	struct dw_pcie *pci = pcie->pci;
>>   	struct device *dev = pci->dev;
>> +	int ret;
>> +
>> +	res->supplies[0].supply = "vdda";
>> +	res->supplies[1].supply = "vdda-1p8";
>> +	res->supplies[2].supply = "vddpe";
>> +	res->supplies[3].supply = "vddpe1";
>> +	res->num_supplies = QCOM_PCIE_MAX_SUPPLY;
>> +	ret = devm_regulator_bulk_get(dev, QCOM_PCIE_MAX_SUPPLY,
>> +				      res->supplies);
> 
> If we decide to go on this direction we need to replace this with
> devm_regulator_bulk_get_optional (yes I know there is no such regulator
> API yet) because they are optional in the DT binding.

I think the api name is misleading here, actually 
devm_regulator_bulk_get() will return dummy regulators if the regulators 
are not present, so code as it is Okay. On the other hand *_optonal api 
would fail.


thanks,
srini
> 
> <snip>
> 

Re: [PATCH] PCI: qcom: add missing supplies required for msm8996

[Stanimir Varbanov]

Hey Srini,

As there are no comments I'd propose to change the endpoint supplies to
more generic names.

On 12/08/2017 11:20 AM, srinivas.kandagatla@linaro.org wrote:
> From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> 
> This patch adds supplies that are required for msm8996. Two of them vdda
> and vdda-1p8 are analog supplies that go in to controller, and the rest
> of the two vddpe's are supplies to PCIe endpoints.
> 
> Without these supplies PCIe endpoints which require power supplies are
> not enumerated at all, as there is no one to power it up.
> 
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  .../devicetree/bindings/pci/qcom,pcie.txt          | 16 +++++++++++++
>  drivers/pci/dwc/pcie-qcom.c                        | 28 ++++++++++++++++++++--
>  2 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.txt b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> index 3c9d321b3d3b..045102cb3e12 100644
> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.txt
> @@ -179,6 +179,11 @@
>  	Value type: <phandle>
>  	Definition: A phandle to the core analog power supply
>  
> +- vdda-1p8-supply:
> +	Usage: required for msm8996
> +	Value type: <phandle>
> +	Definition: A phandle to the 1.8v analog power supply
> +

This should be dropped, because it is part of the phy.

>  - vdda_phy-supply:
>  	Usage: required for ipq/apq8064
>  	Value type: <phandle>
> @@ -189,6 +194,15 @@
>  	Value type: <phandle>
>  	Definition: A phandle to the analog power supply for IC which generates
>  		    reference clock
> +- vddpe-supply:
> +	Usage: optional
> +	Value type: <phandle>
> +	Definition: A phandle to the PCIe endpoint power supply

vddpe_3v3-supply

> +
> +- vddpe1-supply:
> +	Usage: optional
> +	Value type: <phandle>
> +	Definition: A phandle to the PCIe endpoint power supply 1

vddpe_1v5-supply

>  
>  - phys:
>  	Usage: required for apq8084
> @@ -205,6 +219,8 @@
>  	Value type: <prop-encoded-array>
>  	Definition: List of phandle and GPIO specifier pairs. Should contain
>  			- "perst-gpios"	PCIe endpoint reset signal line
> +			- "pe_en-gpios"	PCIe endpoint enable signal line
> +			- "pe_en1-gpios" PCIe endpoint enable1 signal line

We don't need those gpios, the regulator driver will manipulate these
gpios when we call regulator_enable/disable.


-- 
regards,
Stan

Re: [PATCH] PCI: qcom: add missing supplies required for msm8996

[Srinivas Kandagatla]

On 23/01/18 09:23, Stanimir Varbanov wrote:
> Hey Srini,
> 
> As there are no comments I'd propose to change the endpoint supplies to
> more generic names.
> 
Sure, I will respin this with your suggestions, except the 3v3 and 1v5 
suffix due to the reasons below:
>> +- vdda-1p8-supply:
>> +	Usage: required for msm8996
>> +	Value type: <phandle>
>> +	Definition: A phandle to the 1.8v analog power supply
>> +
> 
> This should be dropped, because it is part of the phy.
Yep.

> 
>>   - vdda_phy-supply:
>>   	Usage: required for ipq/apq8064
>>   	Value type: <phandle>
>> @@ -189,6 +194,15 @@
>>   	Value type: <phandle>
>>   	Definition: A phandle to the analog power supply for IC which generates
>>   		    reference clock
>> +- vddpe-supply:
>> +	Usage: optional
>> +	Value type: <phandle>
>> +	Definition: A phandle to the PCIe endpoint power supply
> 
> vddpe_3v3-supply
Why do we need suffix here? AFAIU, It does not add any value, instead it 
would confuse the users.

These are power supplies for endpoint which could be of any voltage. In 
this case both endpoint supplies are 3v3, these could be 1.8 or 5v or 
12v in some other cases.

> 
>> +
>> +- vddpe1-supply:
>> +	Usage: optional
>> +	Value type: <phandle>
>> +	Definition: A phandle to the PCIe endpoint power supply 1
> 
> vddpe_1v5-supply
> 
>>   
>>   - phys:
>>   	Usage: required for apq8084
>> @@ -205,6 +219,8 @@
>>   	Value type: <prop-encoded-array>
>>   	Definition: List of phandle and GPIO specifier pairs. Should contain
>>   			- "perst-gpios"	PCIe endpoint reset signal line
>> +			- "pe_en-gpios"	PCIe endpoint enable signal line
>> +			- "pe_en1-gpios" PCIe endpoint enable1 signal line
> 
> We don't need those gpios, the regulator driver will manipulate these
> gpios when we call regulator_enable/disable.
yes, I will get rid of them.

> 
> 

Re: [PATCH] PCI: qcom: add missing supplies required for msm8996

[Stanimir Varbanov]

Hi,

On 01/23/2018 11:46 AM, Srinivas Kandagatla wrote:
> 
> 
> On 23/01/18 09:23, Stanimir Varbanov wrote:
>> Hey Srini,
>>
>> As there are no comments I'd propose to change the endpoint supplies to
>> more generic names.
>>
> Sure, I will respin this with your suggestions, except the 3v3 and 1v5
> suffix due to the reasons below:
>>> +- vdda-1p8-supply:
>>> +    Usage: required for msm8996
>>> +    Value type: <phandle>
>>> +    Definition: A phandle to the 1.8v analog power supply
>>> +
>>
>> This should be dropped, because it is part of the phy.
> Yep.
> 
>>
>>>   - vdda_phy-supply:
>>>       Usage: required for ipq/apq8064
>>>       Value type: <phandle>
>>> @@ -189,6 +194,15 @@
>>>       Value type: <phandle>
>>>       Definition: A phandle to the analog power supply for IC which
>>> generates
>>>               reference clock
>>> +- vddpe-supply:
>>> +    Usage: optional
>>> +    Value type: <phandle>
>>> +    Definition: A phandle to the PCIe endpoint power supply
>>
>> vddpe_3v3-supply
> Why do we need suffix here? AFAIU, It does not add any value, instead it
> would confuse the users.

vddpe and vddpe1 is already confusing as well.

Lets imagine that powering up the endpointX needs some specific sequence
between 3v3 and 1v5 and endpointY (which could be connected on the same
PCIe lane) has different power sequence, how we would handle that in the
qcom pcie host driver?

> 
> These are power supplies for endpoint which could be of any voltage. In

I don't think that could be any values see PCIe mini card
electromechanical specification. There on the connector are provided 3v3
and 1v5.

> this case both endpoint supplies are 3v3, these could be 1.8 or 5v or
> 12v in some other cases.

If we see hw designs with 5v and 12v we could extend the binding and the
driver with support for them. I want to be exact in the names and
voltages in the driver and bindings.

-- 
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI: qcom: add missing supplies required for msm8996

[Srinivas Kandagatla]

On 23/01/18 10:14, Stanimir Varbanov wrote:
> Hi,
> 
> On 01/23/2018 11:46 AM, Srinivas Kandagatla wrote:
>>
>>
>> On 23/01/18 09:23, Stanimir Varbanov wrote:
>>> Hey Srini,
>>>
>>> As there are no comments I'd propose to change the endpoint supplies to
>>> more generic names.
>>>
>> Sure, I will respin this with your suggestions, except the 3v3 and 1v5
>> suffix due to the reasons below:
>>>> +- vdda-1p8-supply:
>>>> +    Usage: required for msm8996
>>>> +    Value type: <phandle>
>>>> +    Definition: A phandle to the 1.8v analog power supply
>>>> +
>>>
>>> This should be dropped, because it is part of the phy.
>> Yep.
>>
>>>
>>>>    - vdda_phy-supply:
>>>>        Usage: required for ipq/apq8064
>>>>        Value type: <phandle>
>>>> @@ -189,6 +194,15 @@
>>>>        Value type: <phandle>
>>>>        Definition: A phandle to the analog power supply for IC which
>>>> generates
>>>>                reference clock
>>>> +- vddpe-supply:
>>>> +    Usage: optional
>>>> +    Value type: <phandle>
>>>> +    Definition: A phandle to the PCIe endpoint power supply
>>>
>>> vddpe_3v3-supply
>> Why do we need suffix here? AFAIU, It does not add any value, instead it
>> would confuse the users.
> 
> vddpe and vddpe1 is already confusing as well.

I partly agree with you.

How would you represent if there are two power 3v3 supplies for the 
endpoint?

> 
> Lets imagine that powering up the endpointX needs some specific sequence
> between 3v3 and 1v5 and endpointY (which could be connected on the same
> PCIe lane) has different power sequence, how we would handle that in the
> qcom pcie host driver?

power sequencing is all together a different issue, that is not 
addressed in this patch. Am hoping that this will be fixed as part of 
making pwrseq interface more generic. Not sure where it endedup now!!

--srini


> 
>>
>> These are power supplies for endpoint which could be of any voltage. In
> 
> I don't think that could be any values see PCIe mini card
> electromechanical specification. There on the connector are provided 3v3
> and 1v5.
> 
>> this case both endpoint supplies are 3v3, these could be 1.8 or 5v or
>> 12v in some other cases.
> 
> If we see hw designs with 5v and 12v we could extend the binding and the
> driver with support for them. I want to be exact in the names and
> voltages in the driver and bindings.

>