* [PATCH RFC 1/7] dt: bindings: add qcom,qps615.yaml
2024-06-26 12:37 [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Krishna chaitanya chundru
@ 2024-06-26 12:37 ` Krishna chaitanya chundru
2024-06-26 13:34 ` Rob Herring (Arm)
` (3 more replies)
2024-06-26 12:37 ` [PATCH RFC 2/7] arm64: dts: qcom: qcs6490-rb3gen2: Add qps615 node Krishna chaitanya chundru
` (8 subsequent siblings)
9 siblings, 4 replies; 28+ messages in thread
From: Krishna chaitanya chundru @ 2024-06-26 12:37 UTC (permalink / raw)
To: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han
Cc: quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel, Krishna chaitanya chundru
qps615 is a driver for Qualcomm PCIe switch driver which controls
power & configuration of the hardware.
Add a bindings document for the driver.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
.../devicetree/bindings/pci/qcom,qps615.yaml | 73 ++++++++++++++++++++++
1 file changed, 73 insertions(+)
diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
new file mode 100644
index 000000000000..f090683f9e2f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
@@ -0,0 +1,73 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pci/qcom,pcie-qps615.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm QPS615 PCIe switch
+
+maintainers:
+ - Krishna chaitanya chundru <quic_krichai@quicinc.com>
+
+description: |
+ Qualcomm QPS615 PCIe switch has one upstream and three downstream
+ ports. One of the downstream ports is used as endpoint device of
+ Ethernet MAC. Other two downstream ports are supposed to connect
+ to external device.
+
+ The power controlled by the GPIO's, if we enable the GPIO's the
+ power to the switch will be on.
+
+ The QPS615 PCIe switch is configured through I2C interface before
+ PCIe link is established.
+
+properties:
+ compatible:
+ enum:
+ - pci1179,0623
+
+ reg:
+ maxItems: 1
+
+ vdd-supply:
+ description: |
+ Phandle to the vdd input voltage which are fixed regulators which
+ in are mapped to the GPIO's.
+
+ switch-i2c-cntrl:
+ description: |
+ phandle to i2c controller which is used to configure the PCIe
+ switch.
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+ - switch-i2c-cntrl
+
+additionalProperties: false
+
+examples:
+ - |
+ pcie {
+ #address-cells = <3>;
+ #size-cells = <2>;
+
+ pcie@0 {
+ device_type = "pci";
+ reg = <0x0 0x0 0x0 0x0 0x0>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ ranges;
+
+ bus-range = <0x01 0xff>;
+
+ qps615@0 {
+ compatible = "pci1179,0623";
+ reg = <0x10000 0x0 0x0 0x0 0x0>;
+
+ vdd-supply = <&vdd>;
+ switch-i2c-cntrl = <&foo>;
+ };
+ };
+ };
--
2.42.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH RFC 1/7] dt: bindings: add qcom,qps615.yaml
2024-06-26 12:37 ` [PATCH RFC 1/7] dt: bindings: add qcom,qps615.yaml Krishna chaitanya chundru
@ 2024-06-26 13:34 ` Rob Herring (Arm)
2024-06-26 15:01 ` Bjorn Andersson
` (2 subsequent siblings)
3 siblings, 0 replies; 28+ messages in thread
From: Rob Herring (Arm) @ 2024-06-26 13:34 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bjorn Andersson, linux-arm-msm, quic_vbadigan, Konrad Dybcio,
Jingoo Han, devicetree, linux-kernel, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Krzysztof Kozlowski,
linux-pci, Conor Dooley, quic_nitegupt, quic_skananth,
Bjorn Helgaas, Bartosz Golaszewski
On Wed, 26 Jun 2024 18:07:49 +0530, Krishna chaitanya chundru wrote:
> qps615 is a driver for Qualcomm PCIe switch driver which controls
> power & configuration of the hardware.
> Add a bindings document for the driver.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> .../devicetree/bindings/pci/qcom,qps615.yaml | 73 ++++++++++++++++++++++
> 1 file changed, 73 insertions(+)
>
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:
./Documentation/devicetree/bindings/pci/qcom,qps615.yaml:70:1: [error] syntax error: found character '\t' that cannot start any token (syntax)
dtschema/dtc warnings/errors:
make[2]: *** Deleting file 'Documentation/devicetree/bindings/pci/qcom,qps615.example.dts'
Documentation/devicetree/bindings/pci/qcom,qps615.yaml:70:1: found a tab character where an indentation space is expected
make[2]: *** [Documentation/devicetree/bindings/Makefile:26: Documentation/devicetree/bindings/pci/qcom,qps615.example.dts] Error 1
make[2]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/pci/qcom,qps615.yaml:70:1: found a tab character where an indentation space is expected
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pci/qcom,qps615.yaml: ignoring, error parsing file
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1430: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240626-qps615-v1-1-2ade7bd91e02@quicinc.com
The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 1/7] dt: bindings: add qcom,qps615.yaml
2024-06-26 12:37 ` [PATCH RFC 1/7] dt: bindings: add qcom,qps615.yaml Krishna chaitanya chundru
2024-06-26 13:34 ` Rob Herring (Arm)
@ 2024-06-26 15:01 ` Bjorn Andersson
2024-06-27 15:31 ` Rob Herring
2024-07-01 13:26 ` Krzysztof Kozlowski
3 siblings, 0 replies; 28+ messages in thread
From: Bjorn Andersson @ 2024-06-26 15:01 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Konrad Dybcio, Jingoo Han,
quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 06:07:49PM GMT, Krishna chaitanya chundru wrote:
> qps615 is a driver for Qualcomm PCIe switch driver which controls
> power & configuration of the hardware.
> Add a bindings document for the driver.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> .../devicetree/bindings/pci/qcom,qps615.yaml | 73 ++++++++++++++++++++++
> 1 file changed, 73 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> new file mode 100644
> index 000000000000..f090683f9e2f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/qcom,pcie-qps615.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QPS615 PCIe switch
> +
> +maintainers:
> + - Krishna chaitanya chundru <quic_krichai@quicinc.com>
> +
> +description: |
> + Qualcomm QPS615 PCIe switch has one upstream and three downstream
> + ports. One of the downstream ports is used as endpoint device of
> + Ethernet MAC. Other two downstream ports are supposed to connect
> + to external device.
Hopefully this isn't the only possible use of QPS615, so I'd suggest
that you omit the rbg3gen2-specific integration details from this
binding. In other words, describe the QPS615, not the QPS615 in rb3gen2.
> +
> + The power controlled by the GPIO's, if we enable the GPIO's the
> + power to the switch will be on.
> +
> + The QPS615 PCIe switch is configured through I2C interface before
> + PCIe link is established.
> +
> +properties:
> + compatible:
> + enum:
> + - pci1179,0623
> +
> + reg:
> + maxItems: 1
> +
> + vdd-supply:
> + description: |
'|' means "preserve formatting", but there's nothing here to preserve,
please drop it.
> + Phandle to the vdd input voltage which are fixed regulators which
> + in are mapped to the GPIO's.
The binding for a regulator consumer should not document how the
providing regulator is implemented. Only thing to describe here it which
"supply pin" this refers to, which turns this into "vdd input" or "vdd
pin supply" (if that's what the datasheet call this pin on the QPS615)
and as such you can drop the description because the name of the
property already states that.
> +
> + switch-i2c-cntrl:
I'd prefer you call it "i2c-adapter" or perhaps "i2c-bus", because it's
not "the switch controller".
> + description: |
> + phandle to i2c controller which is used to configure the PCIe
> + switch.
"Reference to the i2c adapter/bus used to configure the QPS615"
But I wonder if this somehow should be described on the particular i2c
bus and be referenced from here instead.
It's not obvious how to describe these components that sits on two
different busses - although I believe this binding refers to the
"configuration interface" sitting on the i2c bus, abut it's described on
the PCIe bus as it relates to power sequencing.
> +
> +required:
> + - compatible
> + - reg
> + - vdd-supply
> + - switch-i2c-cntrl
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + pcie {
> + #address-cells = <3>;
> + #size-cells = <2>;
> +
> + pcie@0 {
> + device_type = "pci";
> + reg = <0x0 0x0 0x0 0x0 0x0>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + ranges;
> +
> + bus-range = <0x01 0xff>;
> +
> + qps615@0 {
> + compatible = "pci1179,0623";
> + reg = <0x10000 0x0 0x0 0x0 0x0>;
> +
> + vdd-supply = <&vdd>;
> + switch-i2c-cntrl = <&foo>;
Indentation looks off here.
Regards,
Bjorn
> + };
> + };
> + };
>
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 1/7] dt: bindings: add qcom,qps615.yaml
2024-06-26 12:37 ` [PATCH RFC 1/7] dt: bindings: add qcom,qps615.yaml Krishna chaitanya chundru
2024-06-26 13:34 ` Rob Herring (Arm)
2024-06-26 15:01 ` Bjorn Andersson
@ 2024-06-27 15:31 ` Rob Herring
2024-07-01 13:26 ` Krzysztof Kozlowski
3 siblings, 0 replies; 28+ messages in thread
From: Rob Herring @ 2024-06-27 15:31 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Bjorn Helgaas, Krzysztof Kozlowski,
Conor Dooley, Bjorn Andersson, Konrad Dybcio, Jingoo Han,
quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 06:07:49PM +0530, Krishna chaitanya chundru wrote:
> qps615 is a driver for Qualcomm PCIe switch driver which controls
> power & configuration of the hardware.
> Add a bindings document for the driver.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> .../devicetree/bindings/pci/qcom,qps615.yaml | 73 ++++++++++++++++++++++
> 1 file changed, 73 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/qcom,qps615.yaml b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> new file mode 100644
> index 000000000000..f090683f9e2f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/qcom,qps615.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pci/qcom,pcie-qps615.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm QPS615 PCIe switch
> +
> +maintainers:
> + - Krishna chaitanya chundru <quic_krichai@quicinc.com>
> +
> +description: |
> + Qualcomm QPS615 PCIe switch has one upstream and three downstream
> + ports. One of the downstream ports is used as endpoint device of
> + Ethernet MAC. Other two downstream ports are supposed to connect
> + to external device.
> +
> + The power controlled by the GPIO's, if we enable the GPIO's the
> + power to the switch will be on.
> +
> + The QPS615 PCIe switch is configured through I2C interface before
> + PCIe link is established.
> +
As a PCI device and implementing a PCI bus, you need to reference
pci-pci-bridge.yaml. And you'll need to fix your example when you add
that.
> +properties:
> + compatible:
> + enum:
> + - pci1179,0623
> +
> + reg:
> + maxItems: 1
> +
> + vdd-supply:
> + description: |
> + Phandle to the vdd input voltage which are fixed regulators which
> + in are mapped to the GPIO's.
> +
> + switch-i2c-cntrl:
> + description: |
> + phandle to i2c controller which is used to configure the PCIe
> + switch.
There's a somewhat standard property for this purpose: i2c-bus
> +
> +required:
> + - compatible
> + - reg
> + - vdd-supply
> + - switch-i2c-cntrl
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + pcie {
> + #address-cells = <3>;
> + #size-cells = <2>;
> +
> + pcie@0 {
> + device_type = "pci";
> + reg = <0x0 0x0 0x0 0x0 0x0>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> + ranges;
> +
> + bus-range = <0x01 0xff>;
Unless there's a h/w limitation, you don't need this.
> +
> + qps615@0 {
> + compatible = "pci1179,0623";
> + reg = <0x10000 0x0 0x0 0x0 0x0>;
> +
> + vdd-supply = <&vdd>;
> + switch-i2c-cntrl = <&foo>;
> + };
> + };
> + };
>
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 1/7] dt: bindings: add qcom,qps615.yaml
2024-06-26 12:37 ` [PATCH RFC 1/7] dt: bindings: add qcom,qps615.yaml Krishna chaitanya chundru
` (2 preceding siblings ...)
2024-06-27 15:31 ` Rob Herring
@ 2024-07-01 13:26 ` Krzysztof Kozlowski
3 siblings, 0 replies; 28+ messages in thread
From: Krzysztof Kozlowski @ 2024-07-01 13:26 UTC (permalink / raw)
To: Krishna chaitanya chundru, Bartosz Golaszewski,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han
Cc: quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel
On 26/06/2024 14:37, Krishna chaitanya chundru wrote:
> qps615 is a driver for Qualcomm PCIe switch driver which controls
> power & configuration of the hardware.
> Add a bindings document for the driver.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 2/7] arm64: dts: qcom: qcs6490-rb3gen2: Add qps615 node
2024-06-26 12:37 [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Krishna chaitanya chundru
2024-06-26 12:37 ` [PATCH RFC 1/7] dt: bindings: add qcom,qps615.yaml Krishna chaitanya chundru
@ 2024-06-26 12:37 ` Krishna chaitanya chundru
2024-06-26 17:11 ` Bjorn Andersson
2024-06-26 17:51 ` Dmitry Baryshkov
2024-06-26 12:37 ` [PATCH RFC 3/7] pci: Change the parent of the platform devices for child OF nodes Krishna chaitanya chundru
` (7 subsequent siblings)
9 siblings, 2 replies; 28+ messages in thread
From: Krishna chaitanya chundru @ 2024-06-26 12:37 UTC (permalink / raw)
To: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han
Cc: quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel, Krishna chaitanya chundru
QPS615 switch power is controlled by GPIO's. Once the GPIO's are
enabled, switch power will be on.
Make all GPIO's as fixed regulators and inter link them so that
enabling the regulator will enable power to the switch by enabling
GPIO's.
Enable i2c0 which is required to configure the switch.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 55 ++++++++++++++++++++++++++++
1 file changed, 55 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
index a085ff5b5fb2..5b453896a6c9 100644
--- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
+++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
@@ -511,6 +511,61 @@ vreg_bob_3p296: bob {
regulator-max-microvolt = <3960000>;
};
};
+
+ qps615_0p9_vreg: qps615-0p9-vreg {
+ compatible = "regulator-fixed";
+ regulator-name = "qps615_0p9_vreg";
+ gpio = <&pm8350c_gpios 2 0>;
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1000000>;
+ enable-active-high;
+ regulator-enable-ramp-delay = <4300>;
+ };
+
+ qps615_1p8_vreg: qps615-1p8-vreg {
+ compatible = "regulator-fixed";
+ regulator-name = "qps615_1p8_vreg";
+ gpio = <&pm8350c_gpios 3 0>;
+ vin-supply = <&qps615_0p9_vreg>;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ enable-active-high;
+ regulator-enable-ramp-delay = <10000>;
+ };
+
+ qps615_rsex_vreg: qps615-rsex-vreg {
+ compatible = "regulator-fixed";
+ regulator-name = "qps615_rsex_vreg";
+ gpio = <&pm8350c_gpios 1 0>;
+ vin-supply = <&qps615_1p8_vreg>;
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ enable-active-high;
+ regulator-enable-ramp-delay = <10000>;
+ };
+};
+
+&i2c0 {
+ clock-frequency = <100000>;
+ status = "okay";
+};
+
+&pcie1 {
+ pcie@0 {
+ device_type = "pci";
+ reg = <0x0 0x0 0x0 0x0 0x0>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+
+ bus-range = <0x01 0xff>;
+
+ qps615@0 {
+ compatible = "pci1179,0623";
+ reg = <0x1000 0x0 0x0 0x0 0x0>;
+ vdda-supply = <&qps615_rsex_vreg>;
+ switch-i2c-cntrl = <&i2c0>;
+ };
+ };
};
&gcc {
--
2.42.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH RFC 2/7] arm64: dts: qcom: qcs6490-rb3gen2: Add qps615 node
2024-06-26 12:37 ` [PATCH RFC 2/7] arm64: dts: qcom: qcs6490-rb3gen2: Add qps615 node Krishna chaitanya chundru
@ 2024-06-26 17:11 ` Bjorn Andersson
2024-06-26 17:51 ` Dmitry Baryshkov
1 sibling, 0 replies; 28+ messages in thread
From: Bjorn Andersson @ 2024-06-26 17:11 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Konrad Dybcio, Jingoo Han,
quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 06:07:50PM GMT, Krishna chaitanya chundru wrote:
> QPS615 switch power is controlled by GPIO's. Once the GPIO's are
> enabled, switch power will be on.
>
> Make all GPIO's as fixed regulators and inter link them so that
> enabling the regulator will enable power to the switch by enabling
> GPIO's.
>
> Enable i2c0 which is required to configure the switch.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 55 ++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> index a085ff5b5fb2..5b453896a6c9 100644
> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> @@ -511,6 +511,61 @@ vreg_bob_3p296: bob {
> regulator-max-microvolt = <3960000>;
> };
> };
> +
> + qps615_0p9_vreg: qps615-0p9-vreg {
Keep nodes sorted by address, node name, then label.
Perhaps name the nodes "regulator-<name>" to group static regulators
together.
> + compatible = "regulator-fixed";
> + regulator-name = "qps615_0p9_vreg";
Is this the name used for the output of the regulator in the schematics?
> + gpio = <&pm8350c_gpios 2 0>;
Replace that 0 with GPIO_ACTIVE_HIGH.
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + enable-active-high;
> + regulator-enable-ramp-delay = <4300>;
> + };
> +
> + qps615_1p8_vreg: qps615-1p8-vreg {
> + compatible = "regulator-fixed";
> + regulator-name = "qps615_1p8_vreg";
> + gpio = <&pm8350c_gpios 3 0>;
> + vin-supply = <&qps615_0p9_vreg>;
This makes me curious, &qps615_0p9_vreg provides 1V, which we feed into
another regulator (which typically would be an LDO) to provide 1.8V...
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + enable-active-high;
> + regulator-enable-ramp-delay = <10000>;
> + };
> +
> + qps615_rsex_vreg: qps615-rsex-vreg {
> + compatible = "regulator-fixed";
> + regulator-name = "qps615_rsex_vreg";
> + gpio = <&pm8350c_gpios 1 0>;
> + vin-supply = <&qps615_1p8_vreg>;
...which is then fed to another LDO(?) which provides 1.8V.
Please double check the schematics and make sure this represents what's
on the board. Feel free to ping me offline and I can help review the
schematics.
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + enable-active-high;
> + regulator-enable-ramp-delay = <10000>;
> + };
> +};
> +
> +&i2c0 {
> + clock-frequency = <100000>;
> + status = "okay";
> +};
> +
> +&pcie1 {
> + pcie@0 {
> + device_type = "pci";
> + reg = <0x0 0x0 0x0 0x0 0x0>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> +
> + bus-range = <0x01 0xff>;
> +
> + qps615@0 {
> + compatible = "pci1179,0623";
> + reg = <0x1000 0x0 0x0 0x0 0x0>;
> + vdda-supply = <&qps615_rsex_vreg>;
I presume you didn't "make qcom/qcs6490-rb3gen2.dtb CHECK_DTBS=1" this,
as "vdda-supply" is not listed as a valid supply in the binding (also
the driver is looking for vdd-supply...)
Regards,
Bjorn
> + switch-i2c-cntrl = <&i2c0>;
> + };
> + };
> };
>
> &gcc {
>
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 2/7] arm64: dts: qcom: qcs6490-rb3gen2: Add qps615 node
2024-06-26 12:37 ` [PATCH RFC 2/7] arm64: dts: qcom: qcs6490-rb3gen2: Add qps615 node Krishna chaitanya chundru
2024-06-26 17:11 ` Bjorn Andersson
@ 2024-06-26 17:51 ` Dmitry Baryshkov
1 sibling, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-06-26 17:51 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han, quic_vbadigan, quic_skananth, quic_nitegupt,
linux-arm-msm, linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 06:07:50PM GMT, Krishna chaitanya chundru wrote:
> QPS615 switch power is controlled by GPIO's. Once the GPIO's are
> enabled, switch power will be on.
>
> Make all GPIO's as fixed regulators and inter link them so that
> enabling the regulator will enable power to the switch by enabling
> GPIO's.
>
> Enable i2c0 which is required to configure the switch.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 55 ++++++++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> index a085ff5b5fb2..5b453896a6c9 100644
> --- a/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> +++ b/arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts
> @@ -511,6 +511,61 @@ vreg_bob_3p296: bob {
> regulator-max-microvolt = <3960000>;
> };
> };
> +
> + qps615_0p9_vreg: qps615-0p9-vreg {
> + compatible = "regulator-fixed";
> + regulator-name = "qps615_0p9_vreg";
> + gpio = <&pm8350c_gpios 2 0>;
> + regulator-min-microvolt = <1000000>;
> + regulator-max-microvolt = <1000000>;
> + enable-active-high;
> + regulator-enable-ramp-delay = <4300>;
> + };
> +
> + qps615_1p8_vreg: qps615-1p8-vreg {
> + compatible = "regulator-fixed";
> + regulator-name = "qps615_1p8_vreg";
> + gpio = <&pm8350c_gpios 3 0>;
> + vin-supply = <&qps615_0p9_vreg>;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + enable-active-high;
> + regulator-enable-ramp-delay = <10000>;
> + };
> +
> + qps615_rsex_vreg: qps615-rsex-vreg {
> + compatible = "regulator-fixed";
> + regulator-name = "qps615_rsex_vreg";
> + gpio = <&pm8350c_gpios 1 0>;
> + vin-supply = <&qps615_1p8_vreg>;
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + enable-active-high;
> + regulator-enable-ramp-delay = <10000>;
> + };
> +};
> +
> +&i2c0 {
> + clock-frequency = <100000>;
> + status = "okay";
> +};
> +
> +&pcie1 {
> + pcie@0 {
> + device_type = "pci";
> + reg = <0x0 0x0 0x0 0x0 0x0>;
> + #address-cells = <3>;
> + #size-cells = <2>;
> +
> + bus-range = <0x01 0xff>;
> +
> + qps615@0 {
> + compatible = "pci1179,0623";
> + reg = <0x1000 0x0 0x0 0x0 0x0>;
> + vdda-supply = <&qps615_rsex_vreg>;
> + switch-i2c-cntrl = <&i2c0>;
We already have proper bindings for I2C devices. The QPS615 obviously
has and I2C device inside. Please add it to DT instead of referencing
the whole bus.
> + };
> + };
> };
>
> &gcc {
>
> --
> 2.42.0
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 3/7] pci: Change the parent of the platform devices for child OF nodes
2024-06-26 12:37 [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Krishna chaitanya chundru
2024-06-26 12:37 ` [PATCH RFC 1/7] dt: bindings: add qcom,qps615.yaml Krishna chaitanya chundru
2024-06-26 12:37 ` [PATCH RFC 2/7] arm64: dts: qcom: qcs6490-rb3gen2: Add qps615 node Krishna chaitanya chundru
@ 2024-06-26 12:37 ` Krishna chaitanya chundru
2024-06-26 15:12 ` Bartosz Golaszewski
2024-07-01 17:54 ` Bjorn Helgaas
2024-06-26 12:37 ` [PATCH RFC 4/7] pci: Add new start_link() & stop_link function ops Krishna chaitanya chundru
` (6 subsequent siblings)
9 siblings, 2 replies; 28+ messages in thread
From: Krishna chaitanya chundru @ 2024-06-26 12:37 UTC (permalink / raw)
To: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han
Cc: quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel, Krishna chaitanya chundru
Currently the power control driver is child of pci-pci bridge driver,
this will cause issue when suspend resume is introduced in the pwr
control driver. If the supply is removed to the endpoint in the
power control driver then the config space access initaited by the
pci-pci bridge driver can cause issues like Timeouts.
For this reason change the parent to controller from pci-pci bridge.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
drivers/pci/bus.c | 5 +++--
drivers/pci/pwrctl/core.c | 7 ++++++-
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 3e3517567721..eedab4aabd81 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -335,6 +335,7 @@ void __weak pcibios_bus_add_device(struct pci_dev *pdev) { }
void pci_bus_add_device(struct pci_dev *dev)
{
struct device_node *dn = dev->dev.of_node;
+ struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
int retval;
/*
@@ -356,9 +357,9 @@ void pci_bus_add_device(struct pci_dev *dev)
pci_dev_assign_added(dev, true);
- if (pci_is_bridge(dev)) {
+ if (pci_is_bridge(dev) && host) {
retval = of_platform_populate(dev->dev.of_node, NULL, NULL,
- &dev->dev);
+ host->dev.parent);
if (retval)
pci_err(dev, "failed to populate child OF nodes (%d)\n",
retval);
diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
index feca26ad2f6a..4c0d0f3b15f8 100644
--- a/drivers/pci/pwrctl/core.c
+++ b/drivers/pci/pwrctl/core.c
@@ -10,6 +10,7 @@
#include <linux/pci-pwrctl.h>
#include <linux/property.h>
#include <linux/slab.h>
+#include "../pci.h"
static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
void *data)
@@ -64,18 +65,22 @@ static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
*/
int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl)
{
+ struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(pwrctl->dev->parent->of_node), 0);
int ret;
if (!pwrctl->dev)
return -ENODEV;
+ if (!bus)
+ return -ENODEV;
+
pwrctl->nb.notifier_call = pci_pwrctl_notify;
ret = bus_register_notifier(&pci_bus_type, &pwrctl->nb);
if (ret)
return ret;
pci_lock_rescan_remove();
- pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus);
+ pci_rescan_bus(bus);
pci_unlock_rescan_remove();
return 0;
--
2.42.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH RFC 3/7] pci: Change the parent of the platform devices for child OF nodes
2024-06-26 12:37 ` [PATCH RFC 3/7] pci: Change the parent of the platform devices for child OF nodes Krishna chaitanya chundru
@ 2024-06-26 15:12 ` Bartosz Golaszewski
2024-07-01 17:54 ` Bjorn Helgaas
1 sibling, 0 replies; 28+ messages in thread
From: Bartosz Golaszewski @ 2024-06-26 15:12 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han, quic_vbadigan, quic_skananth, quic_nitegupt,
linux-arm-msm, linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 2:38 PM Krishna chaitanya chundru
<quic_krichai@quicinc.com> wrote:
>
> Currently the power control driver is child of pci-pci bridge driver,
> this will cause issue when suspend resume is introduced in the pwr
> control driver. If the supply is removed to the endpoint in the
> power control driver then the config space access initaited by the
> pci-pci bridge driver can cause issues like Timeouts.
>
> For this reason change the parent to controller from pci-pci bridge.
Newline before trailers please.
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> drivers/pci/bus.c | 5 +++--
> drivers/pci/pwrctl/core.c | 7 ++++++-
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 3e3517567721..eedab4aabd81 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -335,6 +335,7 @@ void __weak pcibios_bus_add_device(struct pci_dev *pdev) { }
> void pci_bus_add_device(struct pci_dev *dev)
> {
> struct device_node *dn = dev->dev.of_node;
> + struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> int retval;
>
> /*
> @@ -356,9 +357,9 @@ void pci_bus_add_device(struct pci_dev *dev)
>
> pci_dev_assign_added(dev, true);
>
> - if (pci_is_bridge(dev)) {
> + if (pci_is_bridge(dev) && host) {
I know I told you to check the return value of pci_find_host_bridge()
in private but now after a second look I see it's just a multi-layer
wrapper around container_of() and it looks like it cannot fail so -
correct me if I'm wrong - this can be dropped after all.
> retval = of_platform_populate(dev->dev.of_node, NULL, NULL,
> - &dev->dev);
> + host->dev.parent);
> if (retval)
> pci_err(dev, "failed to populate child OF nodes (%d)\n",
> retval);
> diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
> index feca26ad2f6a..4c0d0f3b15f8 100644
> --- a/drivers/pci/pwrctl/core.c
> +++ b/drivers/pci/pwrctl/core.c
> @@ -10,6 +10,7 @@
> #include <linux/pci-pwrctl.h>
> #include <linux/property.h>
> #include <linux/slab.h>
New line here, please.
> +#include "../pci.h"
>
> static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
> void *data)
> @@ -64,18 +65,22 @@ static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
> */
> int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl)
> {
> + struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(pwrctl->dev->parent->of_node), 0);
> int ret;
>
> if (!pwrctl->dev)
> return -ENODEV;
>
> + if (!bus)
> + return -ENODEV;
This - on the other hand - can fail, so the check if valid. Could you
assign it and then test it in a single spot for better readability?
Bart
> +
> pwrctl->nb.notifier_call = pci_pwrctl_notify;
> ret = bus_register_notifier(&pci_bus_type, &pwrctl->nb);
> if (ret)
> return ret;
>
> pci_lock_rescan_remove();
> - pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus);
> + pci_rescan_bus(bus);
> pci_unlock_rescan_remove();
>
> return 0;
>
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 3/7] pci: Change the parent of the platform devices for child OF nodes
2024-06-26 12:37 ` [PATCH RFC 3/7] pci: Change the parent of the platform devices for child OF nodes Krishna chaitanya chundru
2024-06-26 15:12 ` Bartosz Golaszewski
@ 2024-07-01 17:54 ` Bjorn Helgaas
1 sibling, 0 replies; 28+ messages in thread
From: Bjorn Helgaas @ 2024-07-01 17:54 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han, quic_vbadigan, quic_skananth, quic_nitegupt,
linux-arm-msm, linux-pci, devicetree, linux-kernel
"Change the parent" basically says what the C code does. Can we say
something more specific about what the *purpose* of changing it is?
On Wed, Jun 26, 2024 at 06:07:51PM +0530, Krishna chaitanya chundru wrote:
> Currently the power control driver is child of pci-pci bridge driver,
> this will cause issue when suspend resume is introduced in the pwr
> control driver. If the supply is removed to the endpoint in the
Use "pwrctl" so we can connect them. Also below.
> power control driver then the config space access initaited by the
> pci-pci bridge driver can cause issues like Timeouts.
s/initaited/initiated/ (or just drop it and say "access by the ...")
> For this reason change the parent to controller from pci-pci bridge.
Add blank line here.
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> drivers/pci/bus.c | 5 +++--
> drivers/pci/pwrctl/core.c | 7 ++++++-
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 3e3517567721..eedab4aabd81 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -335,6 +335,7 @@ void __weak pcibios_bus_add_device(struct pci_dev *pdev) { }
> void pci_bus_add_device(struct pci_dev *dev)
> {
> struct device_node *dn = dev->dev.of_node;
> + struct pci_host_bridge *host = pci_find_host_bridge(dev->bus);
> int retval;
>
> /*
> @@ -356,9 +357,9 @@ void pci_bus_add_device(struct pci_dev *dev)
>
> pci_dev_assign_added(dev, true);
>
> - if (pci_is_bridge(dev)) {
> + if (pci_is_bridge(dev) && host) {
> retval = of_platform_populate(dev->dev.of_node, NULL, NULL,
> - &dev->dev);
> + host->dev.parent);
> if (retval)
> pci_err(dev, "failed to populate child OF nodes (%d)\n",
> retval);
> diff --git a/drivers/pci/pwrctl/core.c b/drivers/pci/pwrctl/core.c
> index feca26ad2f6a..4c0d0f3b15f8 100644
> --- a/drivers/pci/pwrctl/core.c
> +++ b/drivers/pci/pwrctl/core.c
> @@ -10,6 +10,7 @@
> #include <linux/pci-pwrctl.h>
> #include <linux/property.h>
> #include <linux/slab.h>
> +#include "../pci.h"
>
> static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
> void *data)
> @@ -64,18 +65,22 @@ static int pci_pwrctl_notify(struct notifier_block *nb, unsigned long action,
> */
> int pci_pwrctl_device_set_ready(struct pci_pwrctl *pwrctl)
> {
> + struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(pwrctl->dev->parent->of_node), 0);
> int ret;
>
> if (!pwrctl->dev)
> return -ENODEV;
>
> + if (!bus)
> + return -ENODEV;
> +
> pwrctl->nb.notifier_call = pci_pwrctl_notify;
> ret = bus_register_notifier(&pci_bus_type, &pwrctl->nb);
> if (ret)
> return ret;
>
> pci_lock_rescan_remove();
> - pci_rescan_bus(to_pci_dev(pwrctl->dev->parent)->bus);
> + pci_rescan_bus(bus);
> pci_unlock_rescan_remove();
>
> return 0;
>
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 4/7] pci: Add new start_link() & stop_link function ops
2024-06-26 12:37 [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Krishna chaitanya chundru
` (2 preceding siblings ...)
2024-06-26 12:37 ` [PATCH RFC 3/7] pci: Change the parent of the platform devices for child OF nodes Krishna chaitanya chundru
@ 2024-06-26 12:37 ` Krishna chaitanya chundru
2024-06-26 15:07 ` Bartosz Golaszewski
2024-06-26 12:37 ` [PATCH RFC 5/7] pci: dwc: Add support for new pci function op Krishna chaitanya chundru
` (5 subsequent siblings)
9 siblings, 1 reply; 28+ messages in thread
From: Krishna chaitanya chundru @ 2024-06-26 12:37 UTC (permalink / raw)
To: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han
Cc: quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel, Krishna chaitanya chundru
Certain devices like QPS615 which uses PCI pwrctl framework
needs to stop link training before configuring the PCIe device.
As controller driver already enables link training, we need to
stop the link training by using stop_link and enable them back after
device is configured by using start_link.
The stop_link() & start_link() be used to keep the link in D3cold &
D0 before turning off the power of the device.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
include/linux/pci.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index fb004fd4e889..3892ff7fd536 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -803,6 +803,8 @@ struct pci_ops {
void __iomem *(*map_bus)(struct pci_bus *bus, unsigned int devfn, int where);
int (*read)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
+ int (*start_link)(struct pci_bus *bus);
+ int (*stop_link)(struct pci_bus *bus);
};
/*
--
2.42.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH RFC 4/7] pci: Add new start_link() & stop_link function ops
2024-06-26 12:37 ` [PATCH RFC 4/7] pci: Add new start_link() & stop_link function ops Krishna chaitanya chundru
@ 2024-06-26 15:07 ` Bartosz Golaszewski
0 siblings, 0 replies; 28+ messages in thread
From: Bartosz Golaszewski @ 2024-06-26 15:07 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han, quic_vbadigan, quic_skananth, quic_nitegupt,
linux-arm-msm, linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 2:38 PM Krishna chaitanya chundru
<quic_krichai@quicinc.com> wrote:
>
> Certain devices like QPS615 which uses PCI pwrctl framework
> needs to stop link training before configuring the PCIe device.
>
> As controller driver already enables link training, we need to
> stop the link training by using stop_link and enable them back after
> device is configured by using start_link.
>
> The stop_link() & start_link() be used to keep the link in D3cold &
> D0 before turning off the power of the device.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> include/linux/pci.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index fb004fd4e889..3892ff7fd536 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -803,6 +803,8 @@ struct pci_ops {
> void __iomem *(*map_bus)(struct pci_bus *bus, unsigned int devfn, int where);
> int (*read)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val);
> int (*write)(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val);
> + int (*start_link)(struct pci_bus *bus);
> + int (*stop_link)(struct pci_bus *bus);
> };
>
IMO stop_link() should return void.
Bart
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 5/7] pci: dwc: Add support for new pci function op
2024-06-26 12:37 [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Krishna chaitanya chundru
` (3 preceding siblings ...)
2024-06-26 12:37 ` [PATCH RFC 4/7] pci: Add new start_link() & stop_link function ops Krishna chaitanya chundru
@ 2024-06-26 12:37 ` Krishna chaitanya chundru
2024-06-26 12:37 ` [PATCH RFC 6/7] pci: qcom: Add support for start_link() & stop_link() Krishna chaitanya chundru
` (4 subsequent siblings)
9 siblings, 0 replies; 28+ messages in thread
From: Krishna chaitanya chundru @ 2024-06-26 12:37 UTC (permalink / raw)
To: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han
Cc: quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel, Krishna chaitanya chundru
Add the support for stop_link & start_link function op.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
drivers/pci/controller/dwc/pcie-designware-host.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index d15a5c2d5b48..edf624768145 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -646,10 +646,29 @@ void __iomem *dw_pcie_own_conf_map_bus(struct pci_bus *bus, unsigned int devfn,
}
EXPORT_SYMBOL_GPL(dw_pcie_own_conf_map_bus);
+static int dw_pcie_host_start_link(struct pci_bus *bus)
+{
+ struct dw_pcie_rp *pp = bus->sysdata;
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+ return dw_pcie_start_link(pci);
+}
+
+static int dw_pcie_host_stop_link(struct pci_bus *bus)
+{
+ struct dw_pcie_rp *pp = bus->sysdata;
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+
+ dw_pcie_stop_link(pci);
+ return 0;
+}
+
static struct pci_ops dw_pcie_ops = {
.map_bus = dw_pcie_own_conf_map_bus,
.read = pci_generic_config_read,
.write = pci_generic_config_write,
+ .start_link = dw_pcie_host_start_link,
+ .stop_link = dw_pcie_host_stop_link,
};
static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
--
2.42.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH RFC 6/7] pci: qcom: Add support for start_link() & stop_link()
2024-06-26 12:37 [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Krishna chaitanya chundru
` (4 preceding siblings ...)
2024-06-26 12:37 ` [PATCH RFC 5/7] pci: dwc: Add support for new pci function op Krishna chaitanya chundru
@ 2024-06-26 12:37 ` Krishna chaitanya chundru
2024-06-26 14:31 ` Bjorn Andersson
2024-07-01 20:14 ` Bjorn Helgaas
2024-06-26 12:37 ` [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615 Krishna chaitanya chundru
` (3 subsequent siblings)
9 siblings, 2 replies; 28+ messages in thread
From: Krishna chaitanya chundru @ 2024-06-26 12:37 UTC (permalink / raw)
To: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han
Cc: quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel, Krishna chaitanya chundru
In the stop_link() if the PCIe link is not up, disable LTSSM enable
bit to stop link training otherwise keep the link in D3cold.
And in the start_link() the enable LTSSM bit if the resources are
turned on other wise do the all the initialization and then start
the link.
Introduce ltssm_disable function op to stop the link training.
Use a flag 'pci_pwrctl_turned_off" to indicate the resources are
turned off by the pci pwrctl framework.
If the link is stopped using the stop_link() then just return with
doing anything in suspend and resume.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
drivers/pci/controller/dwc/pcie-qcom.c | 108 +++++++++++++++++++++++++++++----
1 file changed, 97 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 14772edcf0d3..1ab3ffdb3914 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -37,6 +37,7 @@
/* PARF registers */
#define PARF_SYS_CTRL 0x00
#define PARF_PM_CTRL 0x20
+#define PARF_PM_STTS 0x24
#define PARF_PCS_DEEMPH 0x34
#define PARF_PCS_SWING 0x38
#define PARF_PHY_CTRL 0x40
@@ -83,6 +84,9 @@
/* PARF_PM_CTRL register fields */
#define REQ_NOT_ENTR_L1 BIT(5)
+/* PARF_PM_STTS register fields */
+#define PM_ENTER_L23 BIT(5)
+
/* PARF_PCS_DEEMPH register fields */
#define PCS_DEEMPH_TX_DEEMPH_GEN1(x) FIELD_PREP(GENMASK(21, 16), x)
#define PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(x) FIELD_PREP(GENMASK(13, 8), x)
@@ -126,6 +130,7 @@
/* ELBI_SYS_CTRL register fields */
#define ELBI_SYS_CTRL_LT_ENABLE BIT(0)
+#define ELBI_SYS_CTRL_PME_TURNOFF_MSG BIT(4)
/* AXI_MSTR_RESP_COMP_CTRL0 register fields */
#define CFG_REMOTE_RD_REQ_BRIDGE_SIZE_2K 0x4
@@ -228,6 +233,7 @@ struct qcom_pcie_ops {
void (*host_post_init)(struct qcom_pcie *pcie);
void (*deinit)(struct qcom_pcie *pcie);
void (*ltssm_enable)(struct qcom_pcie *pcie);
+ void (*ltssm_disable)(struct qcom_pcie *pcie);
int (*config_sid)(struct qcom_pcie *pcie);
};
@@ -248,10 +254,13 @@ struct qcom_pcie {
const struct qcom_pcie_cfg *cfg;
struct dentry *debugfs;
bool suspended;
+ bool pci_pwrctl_turned_off;
};
#define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
+static void qcom_pcie_icc_update(struct qcom_pcie *pcie);
+
static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
{
gpiod_set_value_cansleep(pcie->reset, 1);
@@ -266,17 +275,6 @@ static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
}
-static int qcom_pcie_start_link(struct dw_pcie *pci)
-{
- struct qcom_pcie *pcie = to_qcom_pcie(pci);
-
- /* Enable Link Training state machine */
- if (pcie->cfg->ops->ltssm_enable)
- pcie->cfg->ops->ltssm_enable(pcie);
-
- return 0;
-}
-
static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci)
{
struct qcom_pcie *pcie = to_qcom_pcie(pci);
@@ -556,6 +554,15 @@ static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
return 0;
}
+static void qcom_pcie_2_3_2_ltssm_disable(struct qcom_pcie *pcie)
+{
+ u32 val;
+
+ val = readl(pcie->parf + PARF_LTSSM);
+ val &= ~LTSSM_EN;
+ writel(val, pcie->parf + PARF_LTSSM);
+}
+
static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie)
{
u32 val;
@@ -1336,6 +1343,7 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
.post_init = qcom_pcie_post_init_2_7_0,
.deinit = qcom_pcie_deinit_2_7_0,
.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
+ .ltssm_disable = qcom_pcie_2_3_2_ltssm_disable,
};
/* Qcom IP rev.: 1.9.0 */
@@ -1346,6 +1354,7 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
.host_post_init = qcom_pcie_host_post_init_2_7_0,
.deinit = qcom_pcie_deinit_2_7_0,
.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
+ .ltssm_disable = qcom_pcie_2_3_2_ltssm_disable,
.config_sid = qcom_pcie_config_sid_1_9_0,
};
@@ -1395,9 +1404,81 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
.no_l0s = true,
};
+static int qcom_pcie_turnoff_link(struct dw_pcie *pci)
+{
+ struct qcom_pcie *pcie = to_qcom_pcie(pci);
+ u32 ret_l23, val, ret;
+
+ if (!dw_pcie_link_up(pcie->pci)) {
+ if (pcie->cfg->ops->ltssm_disable)
+ pcie->cfg->ops->ltssm_disable(pcie);
+ } else {
+ writel(ELBI_SYS_CTRL_PME_TURNOFF_MSG, pcie->elbi + ELBI_SYS_CTRL);
+
+ ret_l23 = readl_poll_timeout(pcie->parf + PARF_PM_STTS, val,
+ val & PM_ENTER_L23, 10000, 100000);
+ if (ret_l23) {
+ dev_err(pci->dev, "Failed to enter L2/L3\n");
+ return -ETIMEDOUT;
+ }
+
+ qcom_pcie_host_deinit(&pcie->pci->pp);
+
+ ret = icc_disable(pcie->icc_mem);
+ if (ret)
+ dev_err(pci->dev, "Failed to disable PCIe-MEM interconnect path: %d\n",
+ ret);
+
+ pcie->pci_pwrctl_turned_off = true;
+ }
+
+ return 0;
+}
+
+static int qcom_pcie_turnon_link(struct dw_pcie *pci)
+{
+ struct qcom_pcie *pcie = to_qcom_pcie(pci);
+
+ if (pcie->pci_pwrctl_turned_off) {
+ qcom_pcie_host_init(&pcie->pci->pp);
+
+ dw_pcie_setup_rc(&pcie->pci->pp);
+ }
+
+ if (pcie->cfg->ops->ltssm_enable)
+ pcie->cfg->ops->ltssm_enable(pcie);
+
+ /* Ignore the retval, the devices may come up later. */
+ dw_pcie_wait_for_link(pcie->pci);
+
+ qcom_pcie_icc_update(pcie);
+
+ pcie->pci_pwrctl_turned_off = false;
+
+ return 0;
+}
+
+static int qcom_pcie_start_link(struct dw_pcie *pci)
+{
+ return qcom_pcie_turnon_link(pci);
+}
+
+static void qcom_pcie_stop_link(struct dw_pcie *pci)
+{
+ struct qcom_pcie *pcie = to_qcom_pcie(pci);
+
+ if (!dw_pcie_link_up(pcie->pci)) {
+ if (pcie->cfg->ops->ltssm_disable)
+ pcie->cfg->ops->ltssm_disable(pcie);
+ } else {
+ qcom_pcie_turnoff_link(pci);
+ }
+}
+
static const struct dw_pcie_ops dw_pcie_ops = {
.link_up = qcom_pcie_link_up,
.start_link = qcom_pcie_start_link,
+ .stop_link = qcom_pcie_stop_link,
};
static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
@@ -1604,6 +1685,8 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
struct qcom_pcie *pcie = dev_get_drvdata(dev);
int ret;
+ if (pcie->pci_pwrctl_turned_off)
+ return 0;
/*
* Set minimum bandwidth required to keep data path functional during
* suspend.
@@ -1642,6 +1725,9 @@ static int qcom_pcie_resume_noirq(struct device *dev)
struct qcom_pcie *pcie = dev_get_drvdata(dev);
int ret;
+ if (pcie->pci_pwrctl_turned_off)
+ return 0;
+
if (pcie->suspended) {
ret = qcom_pcie_host_init(&pcie->pci->pp);
if (ret)
--
2.42.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH RFC 6/7] pci: qcom: Add support for start_link() & stop_link()
2024-06-26 12:37 ` [PATCH RFC 6/7] pci: qcom: Add support for start_link() & stop_link() Krishna chaitanya chundru
@ 2024-06-26 14:31 ` Bjorn Andersson
2024-07-01 20:14 ` Bjorn Helgaas
1 sibling, 0 replies; 28+ messages in thread
From: Bjorn Andersson @ 2024-06-26 14:31 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Konrad Dybcio, Jingoo Han,
quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 06:07:54PM GMT, Krishna chaitanya chundru wrote:
Please start your commit message with a description of the problem that
you're solving, then followed by the technical description of your
solution (which you have here).
Also, uppercase "PCI:" in your subject prefix.
> In the stop_link() if the PCIe link is not up, disable LTSSM enable
> bit to stop link training otherwise keep the link in D3cold.
> And in the start_link() the enable LTSSM bit if the resources are
> turned on other wise do the all the initialization and then start
> the link.
>
> Introduce ltssm_disable function op to stop the link training.
>
> Use a flag 'pci_pwrctl_turned_off" to indicate the resources are
> turned off by the pci pwrctl framework.
>
> If the link is stopped using the stop_link() then just return with
> doing anything in suspend and resume.
And an empty line between commit message and the tags.
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 108 +++++++++++++++++++++++++++++----
> 1 file changed, 97 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 14772edcf0d3..1ab3ffdb3914 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -37,6 +37,7 @@
> /* PARF registers */
> #define PARF_SYS_CTRL 0x00
> #define PARF_PM_CTRL 0x20
> +#define PARF_PM_STTS 0x24
> #define PARF_PCS_DEEMPH 0x34
> #define PARF_PCS_SWING 0x38
> #define PARF_PHY_CTRL 0x40
> @@ -83,6 +84,9 @@
> /* PARF_PM_CTRL register fields */
> #define REQ_NOT_ENTR_L1 BIT(5)
>
> +/* PARF_PM_STTS register fields */
> +#define PM_ENTER_L23 BIT(5)
> +
> /* PARF_PCS_DEEMPH register fields */
> #define PCS_DEEMPH_TX_DEEMPH_GEN1(x) FIELD_PREP(GENMASK(21, 16), x)
> #define PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(x) FIELD_PREP(GENMASK(13, 8), x)
> @@ -126,6 +130,7 @@
>
> /* ELBI_SYS_CTRL register fields */
> #define ELBI_SYS_CTRL_LT_ENABLE BIT(0)
> +#define ELBI_SYS_CTRL_PME_TURNOFF_MSG BIT(4)
>
> /* AXI_MSTR_RESP_COMP_CTRL0 register fields */
> #define CFG_REMOTE_RD_REQ_BRIDGE_SIZE_2K 0x4
> @@ -228,6 +233,7 @@ struct qcom_pcie_ops {
> void (*host_post_init)(struct qcom_pcie *pcie);
> void (*deinit)(struct qcom_pcie *pcie);
> void (*ltssm_enable)(struct qcom_pcie *pcie);
> + void (*ltssm_disable)(struct qcom_pcie *pcie);
> int (*config_sid)(struct qcom_pcie *pcie);
> };
>
> @@ -248,10 +254,13 @@ struct qcom_pcie {
> const struct qcom_pcie_cfg *cfg;
> struct dentry *debugfs;
> bool suspended;
> + bool pci_pwrctl_turned_off;
> };
>
> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
>
> +static void qcom_pcie_icc_update(struct qcom_pcie *pcie);
> +
> static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> {
> gpiod_set_value_cansleep(pcie->reset, 1);
> @@ -266,17 +275,6 @@ static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> }
>
> -static int qcom_pcie_start_link(struct dw_pcie *pci)
> -{
> - struct qcom_pcie *pcie = to_qcom_pcie(pci);
> -
> - /* Enable Link Training state machine */
> - if (pcie->cfg->ops->ltssm_enable)
> - pcie->cfg->ops->ltssm_enable(pcie);
> -
> - return 0;
> -}
> -
> static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci)
> {
> struct qcom_pcie *pcie = to_qcom_pcie(pci);
> @@ -556,6 +554,15 @@ static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
> return 0;
> }
>
> +static void qcom_pcie_2_3_2_ltssm_disable(struct qcom_pcie *pcie)
> +{
> + u32 val;
> +
> + val = readl(pcie->parf + PARF_LTSSM);
> + val &= ~LTSSM_EN;
> + writel(val, pcie->parf + PARF_LTSSM);
> +}
> +
> static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie)
> {
> u32 val;
> @@ -1336,6 +1343,7 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
> .post_init = qcom_pcie_post_init_2_7_0,
> .deinit = qcom_pcie_deinit_2_7_0,
> .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> + .ltssm_disable = qcom_pcie_2_3_2_ltssm_disable,
> };
>
> /* Qcom IP rev.: 1.9.0 */
> @@ -1346,6 +1354,7 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> .host_post_init = qcom_pcie_host_post_init_2_7_0,
> .deinit = qcom_pcie_deinit_2_7_0,
> .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> + .ltssm_disable = qcom_pcie_2_3_2_ltssm_disable,
> .config_sid = qcom_pcie_config_sid_1_9_0,
> };
>
> @@ -1395,9 +1404,81 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
> .no_l0s = true,
> };
>
> +static int qcom_pcie_turnoff_link(struct dw_pcie *pci)
> +{
> + struct qcom_pcie *pcie = to_qcom_pcie(pci);
> + u32 ret_l23, val, ret;
> +
> + if (!dw_pcie_link_up(pcie->pci)) {
> + if (pcie->cfg->ops->ltssm_disable)
> + pcie->cfg->ops->ltssm_disable(pcie);
> + } else {
> + writel(ELBI_SYS_CTRL_PME_TURNOFF_MSG, pcie->elbi + ELBI_SYS_CTRL);
> +
> + ret_l23 = readl_poll_timeout(pcie->parf + PARF_PM_STTS, val,
> + val & PM_ENTER_L23, 10000, 100000);
> + if (ret_l23) {
> + dev_err(pci->dev, "Failed to enter L2/L3\n");
> + return -ETIMEDOUT;
> + }
> +
> + qcom_pcie_host_deinit(&pcie->pci->pp);
> +
> + ret = icc_disable(pcie->icc_mem);
> + if (ret)
> + dev_err(pci->dev, "Failed to disable PCIe-MEM interconnect path: %d\n",
> + ret);
> +
> + pcie->pci_pwrctl_turned_off = true;
> + }
> +
> + return 0;
> +}
> +
> +static int qcom_pcie_turnon_link(struct dw_pcie *pci)
> +{
> + struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> + if (pcie->pci_pwrctl_turned_off) {
> + qcom_pcie_host_init(&pcie->pci->pp);
> +
> + dw_pcie_setup_rc(&pcie->pci->pp);
> + }
> +
> + if (pcie->cfg->ops->ltssm_enable)
> + pcie->cfg->ops->ltssm_enable(pcie);
> +
> + /* Ignore the retval, the devices may come up later. */
> + dw_pcie_wait_for_link(pcie->pci);
> +
> + qcom_pcie_icc_update(pcie);
> +
> + pcie->pci_pwrctl_turned_off = false;
> +
> + return 0;
> +}
> +
> +static int qcom_pcie_start_link(struct dw_pcie *pci)
> +{
> + return qcom_pcie_turnon_link(pci);
If you inline qcom_pcie_turnon_link() here, you don't need to have two
different words for "start"/"turnon".
> +}
> +
> +static void qcom_pcie_stop_link(struct dw_pcie *pci)
> +{
> + struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> + if (!dw_pcie_link_up(pcie->pci)) {
Unless I'm reading it wrong, qcom_pcie_turnoff_link() has exactly the
same prologue, so you should be able to just inline
qcom_pcie_turnoff_link() in this function and avoid the "stop"/"turnoff"
naming.
> + if (pcie->cfg->ops->ltssm_disable)
> + pcie->cfg->ops->ltssm_disable(pcie);
> + } else {
> + qcom_pcie_turnoff_link(pci);
> + }
> +}
> +
> static const struct dw_pcie_ops dw_pcie_ops = {
> .link_up = qcom_pcie_link_up,
> .start_link = qcom_pcie_start_link,
> + .stop_link = qcom_pcie_stop_link,
> };
>
> static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> @@ -1604,6 +1685,8 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> struct qcom_pcie *pcie = dev_get_drvdata(dev);
> int ret;
>
This deserves a comment.
> + if (pcie->pci_pwrctl_turned_off)
> + return 0;
> /*
> * Set minimum bandwidth required to keep data path functional during
> * suspend.
> @@ -1642,6 +1725,9 @@ static int qcom_pcie_resume_noirq(struct device *dev)
> struct qcom_pcie *pcie = dev_get_drvdata(dev);
> int ret;
>
Ditto.
> + if (pcie->pci_pwrctl_turned_off)
> + return 0;
> +
Regards,
Bjorn
> if (pcie->suspended) {
> ret = qcom_pcie_host_init(&pcie->pci->pp);
> if (ret)
>
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 6/7] pci: qcom: Add support for start_link() & stop_link()
2024-06-26 12:37 ` [PATCH RFC 6/7] pci: qcom: Add support for start_link() & stop_link() Krishna chaitanya chundru
2024-06-26 14:31 ` Bjorn Andersson
@ 2024-07-01 20:14 ` Bjorn Helgaas
1 sibling, 0 replies; 28+ messages in thread
From: Bjorn Helgaas @ 2024-07-01 20:14 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han, quic_vbadigan, quic_skananth, quic_nitegupt,
linux-arm-msm, linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 06:07:54PM +0530, Krishna chaitanya chundru wrote:
> In the stop_link() if the PCIe link is not up, disable LTSSM enable
> bit to stop link training otherwise keep the link in D3cold.
s/disable LTSSM enable bit/clear LTSSM enable bit/ ?
D3cold is a device state that could apply to a component on the other
end of the link but doesn't apply directly to the link itself, AFAIK.
I assume this would be L2 or L3 for the link.
I think this would be easier to understand if you describe it as "if
the link is up, do something; otherwise disable LTSSM" (similar
comment in the code below).
It would be helpful to explain *why* you want to do this. So far this
basically transcribes the C code but doesn't explain the benefit.
> And in the start_link() the enable LTSSM bit if the resources are
> turned on other wise do the all the initialization and then start
> the link.
>
> Introduce ltssm_disable function op to stop the link training.
>
> Use a flag 'pci_pwrctl_turned_off" to indicate the resources are
> turned off by the pci pwrctl framework.
Match quote style (' vs ").
> If the link is stopped using the stop_link() then just return with
> doing anything in suspend and resume.
Add blank line before signed-off-by.
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> drivers/pci/controller/dwc/pcie-qcom.c | 108 +++++++++++++++++++++++++++++----
> 1 file changed, 97 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 14772edcf0d3..1ab3ffdb3914 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -37,6 +37,7 @@
> /* PARF registers */
> #define PARF_SYS_CTRL 0x00
> #define PARF_PM_CTRL 0x20
> +#define PARF_PM_STTS 0x24
> #define PARF_PCS_DEEMPH 0x34
> #define PARF_PCS_SWING 0x38
> #define PARF_PHY_CTRL 0x40
> @@ -83,6 +84,9 @@
> /* PARF_PM_CTRL register fields */
> #define REQ_NOT_ENTR_L1 BIT(5)
>
> +/* PARF_PM_STTS register fields */
> +#define PM_ENTER_L23 BIT(5)
> +
> /* PARF_PCS_DEEMPH register fields */
> #define PCS_DEEMPH_TX_DEEMPH_GEN1(x) FIELD_PREP(GENMASK(21, 16), x)
> #define PCS_DEEMPH_TX_DEEMPH_GEN2_3_5DB(x) FIELD_PREP(GENMASK(13, 8), x)
> @@ -126,6 +130,7 @@
>
> /* ELBI_SYS_CTRL register fields */
> #define ELBI_SYS_CTRL_LT_ENABLE BIT(0)
> +#define ELBI_SYS_CTRL_PME_TURNOFF_MSG BIT(4)
>
> /* AXI_MSTR_RESP_COMP_CTRL0 register fields */
> #define CFG_REMOTE_RD_REQ_BRIDGE_SIZE_2K 0x4
> @@ -228,6 +233,7 @@ struct qcom_pcie_ops {
> void (*host_post_init)(struct qcom_pcie *pcie);
> void (*deinit)(struct qcom_pcie *pcie);
> void (*ltssm_enable)(struct qcom_pcie *pcie);
> + void (*ltssm_disable)(struct qcom_pcie *pcie);
> int (*config_sid)(struct qcom_pcie *pcie);
> };
>
> @@ -248,10 +254,13 @@ struct qcom_pcie {
> const struct qcom_pcie_cfg *cfg;
> struct dentry *debugfs;
> bool suspended;
> + bool pci_pwrctl_turned_off;
> };
>
> #define to_qcom_pcie(x) dev_get_drvdata((x)->dev)
>
> +static void qcom_pcie_icc_update(struct qcom_pcie *pcie);
> +
> static void qcom_ep_reset_assert(struct qcom_pcie *pcie)
> {
> gpiod_set_value_cansleep(pcie->reset, 1);
> @@ -266,17 +275,6 @@ static void qcom_ep_reset_deassert(struct qcom_pcie *pcie)
> usleep_range(PERST_DELAY_US, PERST_DELAY_US + 500);
> }
>
> -static int qcom_pcie_start_link(struct dw_pcie *pci)
> -{
> - struct qcom_pcie *pcie = to_qcom_pcie(pci);
> -
> - /* Enable Link Training state machine */
> - if (pcie->cfg->ops->ltssm_enable)
> - pcie->cfg->ops->ltssm_enable(pcie);
> -
> - return 0;
> -}
> -
> static void qcom_pcie_clear_aspm_l0s(struct dw_pcie *pci)
> {
> struct qcom_pcie *pcie = to_qcom_pcie(pci);
> @@ -556,6 +554,15 @@ static int qcom_pcie_post_init_1_0_0(struct qcom_pcie *pcie)
> return 0;
> }
>
> +static void qcom_pcie_2_3_2_ltssm_disable(struct qcom_pcie *pcie)
> +{
> + u32 val;
> +
> + val = readl(pcie->parf + PARF_LTSSM);
> + val &= ~LTSSM_EN;
> + writel(val, pcie->parf + PARF_LTSSM);
> +}
> +
> static void qcom_pcie_2_3_2_ltssm_enable(struct qcom_pcie *pcie)
> {
> u32 val;
> @@ -1336,6 +1343,7 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
> .post_init = qcom_pcie_post_init_2_7_0,
> .deinit = qcom_pcie_deinit_2_7_0,
> .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> + .ltssm_disable = qcom_pcie_2_3_2_ltssm_disable,
> };
>
> /* Qcom IP rev.: 1.9.0 */
> @@ -1346,6 +1354,7 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
> .host_post_init = qcom_pcie_host_post_init_2_7_0,
> .deinit = qcom_pcie_deinit_2_7_0,
> .ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
> + .ltssm_disable = qcom_pcie_2_3_2_ltssm_disable,
> .config_sid = qcom_pcie_config_sid_1_9_0,
> };
>
> @@ -1395,9 +1404,81 @@ static const struct qcom_pcie_cfg cfg_sc8280xp = {
> .no_l0s = true,
> };
>
> +static int qcom_pcie_turnoff_link(struct dw_pcie *pci)
> +{
> + struct qcom_pcie *pcie = to_qcom_pcie(pci);
> + u32 ret_l23, val, ret;
> +
> + if (!dw_pcie_link_up(pcie->pci)) {
> + if (pcie->cfg->ops->ltssm_disable)
> + pcie->cfg->ops->ltssm_disable(pcie);
> + } else {
> + writel(ELBI_SYS_CTRL_PME_TURNOFF_MSG, pcie->elbi + ELBI_SYS_CTRL);
> +
> + ret_l23 = readl_poll_timeout(pcie->parf + PARF_PM_STTS, val,
> + val & PM_ENTER_L23, 10000, 100000);
> + if (ret_l23) {
> + dev_err(pci->dev, "Failed to enter L2/L3\n");
> + return -ETIMEDOUT;
> + }
> +
> + qcom_pcie_host_deinit(&pcie->pci->pp);
> +
> + ret = icc_disable(pcie->icc_mem);
> + if (ret)
> + dev_err(pci->dev, "Failed to disable PCIe-MEM interconnect path: %d\n",
> + ret);
> +
> + pcie->pci_pwrctl_turned_off = true;
> + }
Can you invert the condition? It's always a pain to read a negated
condition:
if (dw_pcie_link_up(pcie->pci)) {
...
} else {
if (pcie->cfg->ops->ltssm_disable)
pcie->cfg->ops->ltssm_disable(pcie);
}
Is there any race here between checking for link up and performing the
action? Does anything bad happen if the link goes down after you do
the check but before you do the deinit?
> + return 0;
> +}
> +
> +static int qcom_pcie_turnon_link(struct dw_pcie *pci)
> +{
> + struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> + if (pcie->pci_pwrctl_turned_off) {
> + qcom_pcie_host_init(&pcie->pci->pp);
> +
> + dw_pcie_setup_rc(&pcie->pci->pp);
> + }
> +
> + if (pcie->cfg->ops->ltssm_enable)
> + pcie->cfg->ops->ltssm_enable(pcie);
> +
> + /* Ignore the retval, the devices may come up later. */
> + dw_pcie_wait_for_link(pcie->pci);
> +
> + qcom_pcie_icc_update(pcie);
> +
> + pcie->pci_pwrctl_turned_off = false;
> +
> + return 0;
> +}
> +
> +static int qcom_pcie_start_link(struct dw_pcie *pci)
> +{
> + return qcom_pcie_turnon_link(pci);
> +}
> +
> +static void qcom_pcie_stop_link(struct dw_pcie *pci)
> +{
> + struct qcom_pcie *pcie = to_qcom_pcie(pci);
> +
> + if (!dw_pcie_link_up(pcie->pci)) {
> + if (pcie->cfg->ops->ltssm_disable)
> + pcie->cfg->ops->ltssm_disable(pcie);
> + } else {
> + qcom_pcie_turnoff_link(pci);
> + }
I think this would be easier to read as:
if (dw_pcie_link_up(pcie->pci)) {
qcom_pcie_turnoff_link(pci);
} else {
if (pcie->cfg->ops->ltssm_disable)
pcie->cfg->ops->ltssm_disable(pcie);
}
> +}
> +
> static const struct dw_pcie_ops dw_pcie_ops = {
> .link_up = qcom_pcie_link_up,
> .start_link = qcom_pcie_start_link,
> + .stop_link = qcom_pcie_stop_link,
> };
>
> static int qcom_pcie_icc_init(struct qcom_pcie *pcie)
> @@ -1604,6 +1685,8 @@ static int qcom_pcie_suspend_noirq(struct device *dev)
> struct qcom_pcie *pcie = dev_get_drvdata(dev);
> int ret;
>
> + if (pcie->pci_pwrctl_turned_off)
> + return 0;
> /*
> * Set minimum bandwidth required to keep data path functional during
> * suspend.
> @@ -1642,6 +1725,9 @@ static int qcom_pcie_resume_noirq(struct device *dev)
> struct qcom_pcie *pcie = dev_get_drvdata(dev);
> int ret;
>
> + if (pcie->pci_pwrctl_turned_off)
> + return 0;
> +
> if (pcie->suspended) {
> ret = qcom_pcie_host_init(&pcie->pci->pp);
> if (ret)
>
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615
2024-06-26 12:37 [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Krishna chaitanya chundru
` (5 preceding siblings ...)
2024-06-26 12:37 ` [PATCH RFC 6/7] pci: qcom: Add support for start_link() & stop_link() Krishna chaitanya chundru
@ 2024-06-26 12:37 ` Krishna chaitanya chundru
2024-06-26 15:25 ` Bartosz Golaszewski
` (4 more replies)
2024-06-26 13:31 ` [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Rob Herring (Arm)
` (2 subsequent siblings)
9 siblings, 5 replies; 28+ messages in thread
From: Krishna chaitanya chundru @ 2024-06-26 12:37 UTC (permalink / raw)
To: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han
Cc: quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel, Krishna chaitanya chundru
QPS615 switch needs to configured after powering on and before
PCIe link was up.
As the PCIe controller driver already enables the PCIe link training
at the host side, stop the link training.
Otherwise the moment we turn on the switch it will participate in
the link training and link may come before switch is configured through
i2c.
The switch can be configured different ways like changing de-emphasis
settings of the switch, disabling unused ports etc and these settings
can vary from board to board, for that reason the sequence is taken
from the firmware file which contains the address of the slave, to address
and data to be written to the switch. The driver reads the firmware file
and parses them to apply those configurations to the switch.
Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
---
drivers/pci/pwrctl/Kconfig | 7 +
drivers/pci/pwrctl/Makefile | 1 +
drivers/pci/pwrctl/pci-pwrctl-qps615.c | 278 +++++++++++++++++++++++++++++++++
3 files changed, 286 insertions(+)
diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
index f1b824955d4b..a419b250006d 100644
--- a/drivers/pci/pwrctl/Kconfig
+++ b/drivers/pci/pwrctl/Kconfig
@@ -14,4 +14,11 @@ config PCI_PWRCTL_PWRSEQ
Enable support for the PCI power control driver for device
drivers using the Power Sequencing subsystem.
+config PCI_PWRCTL_QPS615
+ tristate "PCI Power Control driver for QPS615"
+ select PCI_PWRCTL
+ help
+ Enable support for the PCI power control driver for QPS615 and
+ configures it.
+
endmenu
diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile
index d308aae4800c..ac563a70c023 100644
--- a/drivers/pci/pwrctl/Makefile
+++ b/drivers/pci/pwrctl/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_PWRCTL) += pci-pwrctl-core.o
pci-pwrctl-core-y := core.o
obj-$(CONFIG_PCI_PWRCTL_PWRSEQ) += pci-pwrctl-pwrseq.o
+obj-$(CONFIG_PCI_PWRCTL_QPS615) += pci-pwrctl-qps615.o
diff --git a/drivers/pci/pwrctl/pci-pwrctl-qps615.c b/drivers/pci/pwrctl/pci-pwrctl-qps615.c
new file mode 100644
index 000000000000..1f2caf5d7da2
--- /dev/null
+++ b/drivers/pci/pwrctl/pci-pwrctl-qps615.c
@@ -0,0 +1,278 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/device.h>
+#include <linux/firmware.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pci-pwrctl.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/string.h>
+#include <linux/types.h>
+
+#include "../pci.h"
+
+struct qcom_qps615_pwrctl_i2c_setting {
+ u32 slv_addr;
+ u32 reg_addr;
+ u32 val;
+};
+
+struct qcom_qps615_pwrctl_ctx {
+ struct i2c_adapter *adapter;
+ struct pci_pwrctl pwrctl;
+ struct device_link *link;
+ struct regulator *vdd;
+};
+
+/* write 32-bit value to 24 bit register */
+static int qps615_switch_i2c_write(struct qcom_qps615_pwrctl_ctx *ctx, u32 slv_addr, u32 reg_addr,
+ u32 reg_val)
+{
+ struct i2c_msg msg;
+ u8 msg_buf[7];
+ int ret;
+
+ msg.addr = slv_addr;
+ msg.len = 7;
+ msg.flags = 0;
+
+ /* Big Endian for reg addr */
+ msg_buf[0] = (u8)(reg_addr >> 16);
+ msg_buf[1] = (u8)(reg_addr >> 8);
+ msg_buf[2] = (u8)reg_addr;
+
+ /* Little Endian for reg val */
+ msg_buf[3] = (u8)(reg_val);
+ msg_buf[4] = (u8)(reg_val >> 8);
+ msg_buf[5] = (u8)(reg_val >> 16);
+ msg_buf[6] = (u8)(reg_val >> 24);
+
+ msg.buf = msg_buf;
+ ret = i2c_transfer(ctx->adapter, &msg, 1);
+ return ret == 1 ? 0 : ret;
+}
+
+/* read 32 bit value from 24 bit reg addr */
+static int qps615_switch_i2c_read(struct qcom_qps615_pwrctl_ctx *ctx, u32 slv_addr, u32 reg_addr,
+ u32 *reg_val)
+{
+ u8 wr_data[3], rd_data[4] = {0};
+ struct i2c_msg msg[2];
+ int ret;
+
+ msg[0].addr = slv_addr;
+ msg[0].len = 3;
+ msg[0].flags = 0;
+
+ /* Big Endian for reg addr */
+ wr_data[0] = (u8)(reg_addr >> 16);
+ wr_data[1] = (u8)(reg_addr >> 8);
+ wr_data[2] = (u8)reg_addr;
+
+ msg[0].buf = wr_data;
+
+ msg[1].addr = slv_addr;
+ msg[1].len = 4;
+ msg[1].flags = I2C_M_RD;
+
+ msg[1].buf = rd_data;
+
+ ret = i2c_transfer(ctx->adapter, &msg[0], 2);
+ if (ret != 2)
+ return ret;
+
+ *reg_val = (rd_data[3] << 24) | (rd_data[2] << 16) | (rd_data[1] << 8) | rd_data[0];
+
+ return 0;
+}
+
+static int qcom_qps615_pwrctl_init(struct qcom_qps615_pwrctl_ctx *ctx)
+{
+ struct device *dev = ctx->pwrctl.dev;
+ struct qcom_qps615_pwrctl_i2c_setting *set;
+ const struct firmware *fw;
+ const u8 *pos, *eof;
+ int ret;
+ u32 val;
+
+ ret = request_firmware(&fw, "qcom/qps615.bin", dev);
+ if (ret < 0) {
+ dev_err(dev, "firmware loading failed with ret %d\n", ret);
+ return ret;
+ }
+
+ if (!fw) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ pos = fw->data;
+ eof = fw->data + fw->size;
+
+ while (pos < (fw->data + fw->size)) {
+ set = (struct qcom_qps615_pwrctl_i2c_setting *)pos;
+
+ ret = qps615_switch_i2c_write(ctx, set->slv_addr, set->reg_addr, set->val);
+ if (ret) {
+ dev_err(dev,
+ "I2c write failed for slv addr:%x at addr%x with val %x ret %d\n",
+ set->slv_addr, set->reg_addr, set->val, ret);
+ goto err;
+ }
+
+ ret = qps615_switch_i2c_read(ctx, set->slv_addr, set->reg_addr, &val);
+ if (ret) {
+ dev_err(dev, "I2c read failed for slv addr:%x at addr%x ret %d\n",
+ set->slv_addr, set->reg_addr, ret);
+ goto err;
+ }
+
+ if (set->val != val) {
+ dev_err(dev,
+ "I2c read's mismatch for slv:%x at addr%x exp%d got%d\n",
+ set->slv_addr, set->reg_addr, set->val, val);
+ goto err;
+ }
+ pos += sizeof(struct qcom_qps615_pwrctl_i2c_setting);
+ }
+
+err:
+ release_firmware(fw);
+
+ return ret;
+}
+
+static int qcom_qps615_power_on(struct qcom_qps615_pwrctl_ctx *ctx)
+{
+ int ret;
+
+ ret = regulator_enable(ctx->vdd);
+ if (ret) {
+ dev_err(ctx->pwrctl.dev, "cannot enable vdda regulator\n");
+ return ret;
+ }
+
+ ret = qcom_qps615_pwrctl_init(ctx);
+ if (ret)
+ regulator_disable(ctx->vdd);
+
+ return ret;
+}
+
+static int qcom_qps615_power_off(struct qcom_qps615_pwrctl_ctx *ctx)
+{
+ return regulator_disable(ctx->vdd);
+}
+
+static int qcom_qps615_pwrctl_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *node = pdev->dev.of_node;
+ struct qcom_qps615_pwrctl_ctx *ctx;
+ struct device_node *adapter_node;
+ struct pci_host_bridge *bridge;
+ struct i2c_adapter *adapter;
+ struct pci_bus *bus;
+
+ bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
+ if (!bus)
+ return -ENODEV;
+
+ bridge = pci_find_host_bridge(bus);
+
+ ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+ if (!ctx)
+ return -ENOMEM;
+
+ adapter_node = of_parse_phandle(node, "switch-i2c-cntrl", 0);
+ if (adapter_node) {
+ adapter = of_get_i2c_adapter_by_node(adapter_node);
+ __free(adapter_node);
+ if (!adapter)
+ return dev_err_probe(dev, -EPROBE_DEFER,
+ "failed to parse switch-i2c-cntrl\n");
+ }
+
+ ctx->pwrctl.dev = dev;
+ ctx->adapter = adapter;
+ ctx->vdd = devm_regulator_get(dev, "vdd");
+ if (IS_ERR(ctx->vdd))
+ return dev_err_probe(dev, PTR_ERR(ctx->vdd),
+ "failed to get vdd regulator\n");
+
+ ctx->link = device_link_add(&bridge->dev, dev, DL_FLAG_AUTOREMOVE_CONSUMER);
+
+ platform_set_drvdata(pdev, ctx);
+
+ bridge->ops->stop_link(bus);
+ qcom_qps615_power_on(ctx);
+ bridge->ops->start_link(bus);
+
+ return devm_pci_pwrctl_device_set_ready(dev, &ctx->pwrctl);
+}
+
+static const struct of_device_id qcom_qps615_pwrctl_of_match[] = {
+ {
+ .compatible = "pci1179,0623",
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(of, qcom_qps615_pwrctl_of_match);
+
+static int pci_pwrctl_suspend_noirq(struct device *dev)
+{
+ struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
+ struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
+ struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
+
+ bridge->ops->stop_link(bus);
+ qcom_qps615_power_off(ctx);
+
+ return 0;
+}
+
+static int pci_pwrctl_resume_noirq(struct device *dev)
+{
+ struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
+ struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
+ struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
+
+ qcom_qps615_power_on(ctx);
+ bridge->ops->start_link(bus);
+
+ return 0;
+}
+
+static void qcom_qps615_pwrctl_remove(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
+
+ device_link_del(ctx->link);
+ pci_pwrctl_suspend_noirq(dev);
+}
+
+static const struct dev_pm_ops pci_pwrctl_pm_ops = {
+ NOIRQ_SYSTEM_SLEEP_PM_OPS(pci_pwrctl_suspend_noirq, pci_pwrctl_resume_noirq)
+};
+
+static struct platform_driver qcom_qps615_pwrctl_driver = {
+ .driver = {
+ .name = "pwrctl-qps615",
+ .of_match_table = qcom_qps615_pwrctl_of_match,
+ .pm = &pci_pwrctl_pm_ops,
+ },
+ .probe = qcom_qps615_pwrctl_probe,
+ .remove_new = qcom_qps615_pwrctl_remove,
+};
+module_platform_driver(qcom_qps615_pwrctl_driver);
+
+MODULE_AUTHOR("Krishna chaitanya chundru <quic_krichai@quicinc.com>");
+MODULE_DESCRIPTION("Qualcomm QPS615 power control driver");
+MODULE_LICENSE("GPL");
--
2.42.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615
2024-06-26 12:37 ` [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615 Krishna chaitanya chundru
@ 2024-06-26 15:25 ` Bartosz Golaszewski
2024-06-26 15:37 ` Konrad Dybcio
` (3 subsequent siblings)
4 siblings, 0 replies; 28+ messages in thread
From: Bartosz Golaszewski @ 2024-06-26 15:25 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han, quic_vbadigan, quic_skananth, quic_nitegupt,
linux-arm-msm, linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 2:38 PM Krishna chaitanya chundru
<quic_krichai@quicinc.com> wrote:
>
> QPS615 switch needs to configured after powering on and before
> PCIe link was up.
>
> As the PCIe controller driver already enables the PCIe link training
> at the host side, stop the link training.
> Otherwise the moment we turn on the switch it will participate in
> the link training and link may come before switch is configured through
> i2c.
>
> The switch can be configured different ways like changing de-emphasis
> settings of the switch, disabling unused ports etc and these settings
> can vary from board to board, for that reason the sequence is taken
> from the firmware file which contains the address of the slave, to address
> and data to be written to the switch. The driver reads the firmware file
> and parses them to apply those configurations to the switch.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> drivers/pci/pwrctl/Kconfig | 7 +
> drivers/pci/pwrctl/Makefile | 1 +
> drivers/pci/pwrctl/pci-pwrctl-qps615.c | 278 +++++++++++++++++++++++++++++++++
> 3 files changed, 286 insertions(+)
>
> diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
> index f1b824955d4b..a419b250006d 100644
> --- a/drivers/pci/pwrctl/Kconfig
> +++ b/drivers/pci/pwrctl/Kconfig
> @@ -14,4 +14,11 @@ config PCI_PWRCTL_PWRSEQ
> Enable support for the PCI power control driver for device
> drivers using the Power Sequencing subsystem.
>
> +config PCI_PWRCTL_QPS615
> + tristate "PCI Power Control driver for QPS615"
> + select PCI_PWRCTL
> + help
> + Enable support for the PCI power control driver for QPS615 and
> + configures it.
> +
> endmenu
> diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile
> index d308aae4800c..ac563a70c023 100644
> --- a/drivers/pci/pwrctl/Makefile
> +++ b/drivers/pci/pwrctl/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_PWRCTL) += pci-pwrctl-core.o
> pci-pwrctl-core-y := core.o
>
> obj-$(CONFIG_PCI_PWRCTL_PWRSEQ) += pci-pwrctl-pwrseq.o
> +obj-$(CONFIG_PCI_PWRCTL_QPS615) += pci-pwrctl-qps615.o
> diff --git a/drivers/pci/pwrctl/pci-pwrctl-qps615.c b/drivers/pci/pwrctl/pci-pwrctl-qps615.c
> new file mode 100644
> index 000000000000..1f2caf5d7da2
> --- /dev/null
> +++ b/drivers/pci/pwrctl/pci-pwrctl-qps615.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
2024?
> + */
> +
> +#include <linux/device.h>
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pci-pwrctl.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +#include "../pci.h"
> +
> +struct qcom_qps615_pwrctl_i2c_setting {
> + u32 slv_addr;
> + u32 reg_addr;
> + u32 val;
> +};
> +
> +struct qcom_qps615_pwrctl_ctx {
> + struct i2c_adapter *adapter;
> + struct pci_pwrctl pwrctl;
> + struct device_link *link;
> + struct regulator *vdd;
> +};
> +
> +/* write 32-bit value to 24 bit register */
> +static int qps615_switch_i2c_write(struct qcom_qps615_pwrctl_ctx *ctx, u32 slv_addr, u32 reg_addr,
Break the line after "*ctx" unless you have a good reason for long lines?
> + u32 reg_val)
> +{
> + struct i2c_msg msg;
> + u8 msg_buf[7];
> + int ret;
> +
> + msg.addr = slv_addr;
> + msg.len = 7;
> + msg.flags = 0;
> +
> + /* Big Endian for reg addr */
> + msg_buf[0] = (u8)(reg_addr >> 16);
> + msg_buf[1] = (u8)(reg_addr >> 8);
> + msg_buf[2] = (u8)reg_addr;
> +
> + /* Little Endian for reg val */
> + msg_buf[3] = (u8)(reg_val);
> + msg_buf[4] = (u8)(reg_val >> 8);
> + msg_buf[5] = (u8)(reg_val >> 16);
> + msg_buf[6] = (u8)(reg_val >> 24);
> +
> + msg.buf = msg_buf;
> + ret = i2c_transfer(ctx->adapter, &msg, 1);
> + return ret == 1 ? 0 : ret;
> +}
> +
> +/* read 32 bit value from 24 bit reg addr */
> +static int qps615_switch_i2c_read(struct qcom_qps615_pwrctl_ctx *ctx, u32 slv_addr, u32 reg_addr,
> + u32 *reg_val)
> +{
> + u8 wr_data[3], rd_data[4] = {0};
> + struct i2c_msg msg[2];
> + int ret;
> +
> + msg[0].addr = slv_addr;
> + msg[0].len = 3;
> + msg[0].flags = 0;
> +
> + /* Big Endian for reg addr */
> + wr_data[0] = (u8)(reg_addr >> 16);
> + wr_data[1] = (u8)(reg_addr >> 8);
> + wr_data[2] = (u8)reg_addr;
> +
> + msg[0].buf = wr_data;
> +
> + msg[1].addr = slv_addr;
> + msg[1].len = 4;
> + msg[1].flags = I2C_M_RD;
> +
> + msg[1].buf = rd_data;
> +
> + ret = i2c_transfer(ctx->adapter, &msg[0], 2);
> + if (ret != 2)
> + return ret;
> +
> + *reg_val = (rd_data[3] << 24) | (rd_data[2] << 16) | (rd_data[1] << 8) | rd_data[0];
> +
> + return 0;
> +}
> +
> +static int qcom_qps615_pwrctl_init(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> + struct device *dev = ctx->pwrctl.dev;
> + struct qcom_qps615_pwrctl_i2c_setting *set;
> + const struct firmware *fw;
> + const u8 *pos, *eof;
> + int ret;
> + u32 val;
> +
> + ret = request_firmware(&fw, "qcom/qps615.bin", dev);
> + if (ret < 0) {
> + dev_err(dev, "firmware loading failed with ret %d\n", ret);
> + return ret;
> + }
> +
> + if (!fw) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + pos = fw->data;
> + eof = fw->data + fw->size;
> +
> + while (pos < (fw->data + fw->size)) {
> + set = (struct qcom_qps615_pwrctl_i2c_setting *)pos;
> +
> + ret = qps615_switch_i2c_write(ctx, set->slv_addr, set->reg_addr, set->val);
> + if (ret) {
> + dev_err(dev,
> + "I2c write failed for slv addr:%x at addr%x with val %x ret %d\n",
> + set->slv_addr, set->reg_addr, set->val, ret);
> + goto err;
> + }
> +
> + ret = qps615_switch_i2c_read(ctx, set->slv_addr, set->reg_addr, &val);
> + if (ret) {
> + dev_err(dev, "I2c read failed for slv addr:%x at addr%x ret %d\n",
> + set->slv_addr, set->reg_addr, ret);
> + goto err;
> + }
> +
> + if (set->val != val) {
> + dev_err(dev,
> + "I2c read's mismatch for slv:%x at addr%x exp%d got%d\n",
> + set->slv_addr, set->reg_addr, set->val, val);
> + goto err;
> + }
> + pos += sizeof(struct qcom_qps615_pwrctl_i2c_setting);
> + }
> +
> +err:
> + release_firmware(fw);
> +
> + return ret;
> +}
> +
> +static int qcom_qps615_power_on(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> + int ret;
> +
> + ret = regulator_enable(ctx->vdd);
> + if (ret) {
> + dev_err(ctx->pwrctl.dev, "cannot enable vdda regulator\n");
Use dev_err_probe() here and wherever applicable.
> + return ret;
> + }
> +
> + ret = qcom_qps615_pwrctl_init(ctx);
> + if (ret)
> + regulator_disable(ctx->vdd);
> +
> + return ret;
> +}
> +
> +static int qcom_qps615_power_off(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> + return regulator_disable(ctx->vdd);
> +}
> +
> +static int qcom_qps615_pwrctl_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *node = pdev->dev.of_node;
> + struct qcom_qps615_pwrctl_ctx *ctx;
> + struct device_node *adapter_node;
> + struct pci_host_bridge *bridge;
> + struct i2c_adapter *adapter;
> + struct pci_bus *bus;
> +
> + bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> + if (!bus)
> + return -ENODEV;
> +
> + bridge = pci_find_host_bridge(bus);
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + adapter_node = of_parse_phandle(node, "switch-i2c-cntrl", 0);
> + if (adapter_node) {
> + adapter = of_get_i2c_adapter_by_node(adapter_node);
> + __free(adapter_node);
How did this even compile??! I think you meant:
struct device_node *adapter_node __free(device_node) = ... ?
Bart
> + if (!adapter)
> + return dev_err_probe(dev, -EPROBE_DEFER,
> + "failed to parse switch-i2c-cntrl\n");
> + }
> +
> + ctx->pwrctl.dev = dev;
> + ctx->adapter = adapter;
> + ctx->vdd = devm_regulator_get(dev, "vdd");
> + if (IS_ERR(ctx->vdd))
> + return dev_err_probe(dev, PTR_ERR(ctx->vdd),
> + "failed to get vdd regulator\n");
> +
> + ctx->link = device_link_add(&bridge->dev, dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> +
> + platform_set_drvdata(pdev, ctx);
> +
> + bridge->ops->stop_link(bus);
> + qcom_qps615_power_on(ctx);
> + bridge->ops->start_link(bus);
> +
> + return devm_pci_pwrctl_device_set_ready(dev, &ctx->pwrctl);
> +}
> +
> +static const struct of_device_id qcom_qps615_pwrctl_of_match[] = {
> + {
> + .compatible = "pci1179,0623",
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_qps615_pwrctl_of_match);
> +
> +static int pci_pwrctl_suspend_noirq(struct device *dev)
> +{
> + struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> + struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> + struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> + bridge->ops->stop_link(bus);
> + qcom_qps615_power_off(ctx);
> +
> + return 0;
> +}
> +
> +static int pci_pwrctl_resume_noirq(struct device *dev)
> +{
> + struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> + struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> + struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> + qcom_qps615_power_on(ctx);
> + bridge->ops->start_link(bus);
> +
> + return 0;
> +}
> +
> +static void qcom_qps615_pwrctl_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> + device_link_del(ctx->link);
> + pci_pwrctl_suspend_noirq(dev);
> +}
> +
> +static const struct dev_pm_ops pci_pwrctl_pm_ops = {
> + NOIRQ_SYSTEM_SLEEP_PM_OPS(pci_pwrctl_suspend_noirq, pci_pwrctl_resume_noirq)
> +};
> +
> +static struct platform_driver qcom_qps615_pwrctl_driver = {
> + .driver = {
> + .name = "pwrctl-qps615",
> + .of_match_table = qcom_qps615_pwrctl_of_match,
> + .pm = &pci_pwrctl_pm_ops,
> + },
> + .probe = qcom_qps615_pwrctl_probe,
> + .remove_new = qcom_qps615_pwrctl_remove,
> +};
> +module_platform_driver(qcom_qps615_pwrctl_driver);
> +
> +MODULE_AUTHOR("Krishna chaitanya chundru <quic_krichai@quicinc.com>");
> +MODULE_DESCRIPTION("Qualcomm QPS615 power control driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615
2024-06-26 12:37 ` [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615 Krishna chaitanya chundru
2024-06-26 15:25 ` Bartosz Golaszewski
@ 2024-06-26 15:37 ` Konrad Dybcio
2024-08-03 3:18 ` Krishna Chaitanya Chundru
2024-06-26 17:00 ` Bjorn Andersson
` (2 subsequent siblings)
4 siblings, 1 reply; 28+ messages in thread
From: Konrad Dybcio @ 2024-06-26 15:37 UTC (permalink / raw)
To: Krishna chaitanya chundru, Bartosz Golaszewski,
Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Jingoo Han
Cc: quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel
On 26.06.2024 2:37 PM, Krishna chaitanya chundru wrote:
> QPS615 switch needs to configured after powering on and before
> PCIe link was up.
>
> As the PCIe controller driver already enables the PCIe link training
> at the host side, stop the link training.
> Otherwise the moment we turn on the switch it will participate in
> the link training and link may come before switch is configured through
> i2c.
>
> The switch can be configured different ways like changing de-emphasis
> settings of the switch, disabling unused ports etc and these settings
> can vary from board to board, for that reason the sequence is taken
> from the firmware file which contains the address of the slave, to address
> and data to be written to the switch. The driver reads the firmware file
> and parses them to apply those configurations to the switch.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
[...]
> +static int qcom_qps615_pwrctl_init(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> + struct device *dev = ctx->pwrctl.dev;
> + struct qcom_qps615_pwrctl_i2c_setting *set;
> + const struct firmware *fw;
> + const u8 *pos, *eof;
> + int ret;
> + u32 val;
> +
> + ret = request_firmware(&fw, "qcom/qps615.bin", dev);
Is this driver only going to serve one model of the device, that will use
this specific firmware file, ever?
In other words, is QPS615 super special and no other chip like it will be
ever made?
[...]
> +
> + bridge->ops->stop_link(bus);
This is turbo intrusive. What if there are more devices on this bus?
Konrad
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615
2024-06-26 15:37 ` Konrad Dybcio
@ 2024-08-03 3:18 ` Krishna Chaitanya Chundru
0 siblings, 0 replies; 28+ messages in thread
From: Krishna Chaitanya Chundru @ 2024-08-03 3:18 UTC (permalink / raw)
To: Konrad Dybcio, Bartosz Golaszewski, Manivannan Sadhasivam,
Lorenzo Pieralisi, Krzysztof Wilczyński, Rob Herring,
Bjorn Helgaas, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Jingoo Han
Cc: quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel
On 6/26/2024 9:07 PM, Konrad Dybcio wrote:
> On 26.06.2024 2:37 PM, Krishna chaitanya chundru wrote:
>> QPS615 switch needs to configured after powering on and before
>> PCIe link was up.
>>
>> As the PCIe controller driver already enables the PCIe link training
>> at the host side, stop the link training.
>> Otherwise the moment we turn on the switch it will participate in
>> the link training and link may come before switch is configured through
>> i2c.
>>
>> The switch can be configured different ways like changing de-emphasis
>> settings of the switch, disabling unused ports etc and these settings
>> can vary from board to board, for that reason the sequence is taken
>> from the firmware file which contains the address of the slave, to address
>> and data to be written to the switch. The driver reads the firmware file
>> and parses them to apply those configurations to the switch.
>>
>> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
>> ---
>
> [...]
>
>> +static int qcom_qps615_pwrctl_init(struct qcom_qps615_pwrctl_ctx *ctx)
>> +{
>> + struct device *dev = ctx->pwrctl.dev;
>> + struct qcom_qps615_pwrctl_i2c_setting *set;
>> + const struct firmware *fw;
>> + const u8 *pos, *eof;
>> + int ret;
>> + u32 val;
>> +
>> + ret = request_firmware(&fw, "qcom/qps615.bin", dev);
>
> Is this driver only going to serve one model of the device, that will use
> this specific firmware file, ever?
>
> In other words, is QPS615 super special and no other chip like it will be
> ever made?
>
> [...]
>
>> +
>> + bridge->ops->stop_link(bus);
>
> This is turbo intrusive. What if there are more devices on this bus?
>
The expectation of this API from the controller driver is to stop link
training only when the link is not up.
- krishna chaitanya.
> Konrad
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615
2024-06-26 12:37 ` [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615 Krishna chaitanya chundru
2024-06-26 15:25 ` Bartosz Golaszewski
2024-06-26 15:37 ` Konrad Dybcio
@ 2024-06-26 17:00 ` Bjorn Andersson
2024-06-26 18:09 ` Dmitry Baryshkov
2024-07-01 19:54 ` Bjorn Helgaas
4 siblings, 0 replies; 28+ messages in thread
From: Bjorn Andersson @ 2024-06-26 17:00 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Konrad Dybcio, Jingoo Han,
quic_vbadigan, quic_skananth, quic_nitegupt, linux-arm-msm,
linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 06:07:55PM GMT, Krishna chaitanya chundru wrote:
> QPS615 switch needs to configured after powering on and before
> PCIe link was up.
"before PCIe link is brought up"?
>
> As the PCIe controller driver already enables the PCIe link training
> at the host side, stop the link training.
> Otherwise the moment we turn on the switch it will participate in
> the link training and link may come before switch is configured through
> i2c.
>
> The switch can be configured different ways like changing de-emphasis
> settings of the switch, disabling unused ports etc and these settings
> can vary from board to board, for that reason the sequence is taken
> from the firmware file which contains the address of the slave, to address
> and data to be written to the switch. The driver reads the firmware file
> and parses them to apply those configurations to the switch.
>
So the firmware file contains calibration data only? How big is this,
should/could it be provided in driver with calibration settings from
DeviceTree?
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> drivers/pci/pwrctl/Kconfig | 7 +
> drivers/pci/pwrctl/Makefile | 1 +
> drivers/pci/pwrctl/pci-pwrctl-qps615.c | 278 +++++++++++++++++++++++++++++++++
> 3 files changed, 286 insertions(+)
>
> diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
> index f1b824955d4b..a419b250006d 100644
> --- a/drivers/pci/pwrctl/Kconfig
> +++ b/drivers/pci/pwrctl/Kconfig
> @@ -14,4 +14,11 @@ config PCI_PWRCTL_PWRSEQ
> Enable support for the PCI power control driver for device
> drivers using the Power Sequencing subsystem.
>
> +config PCI_PWRCTL_QPS615
> + tristate "PCI Power Control driver for QPS615"
> + select PCI_PWRCTL
> + help
> + Enable support for the PCI power control driver for QPS615 and
> + configures it.
> +
> endmenu
> diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile
> index d308aae4800c..ac563a70c023 100644
> --- a/drivers/pci/pwrctl/Makefile
> +++ b/drivers/pci/pwrctl/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_PWRCTL) += pci-pwrctl-core.o
> pci-pwrctl-core-y := core.o
>
> obj-$(CONFIG_PCI_PWRCTL_PWRSEQ) += pci-pwrctl-pwrseq.o
> +obj-$(CONFIG_PCI_PWRCTL_QPS615) += pci-pwrctl-qps615.o
> diff --git a/drivers/pci/pwrctl/pci-pwrctl-qps615.c b/drivers/pci/pwrctl/pci-pwrctl-qps615.c
> new file mode 100644
> index 000000000000..1f2caf5d7da2
> --- /dev/null
> +++ b/drivers/pci/pwrctl/pci-pwrctl-qps615.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pci-pwrctl.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +#include "../pci.h"
> +
> +struct qcom_qps615_pwrctl_i2c_setting {
> + u32 slv_addr;
Prefer this to be named e.g. device_addr
Or perhaps use "addr", "offset", and "value"?
> + u32 reg_addr;
> + u32 val;
> +};
This struct is typecast straight from the byte array provided by
request_firmware(), it would be preferable to use endian-specific data
types to highlight this: i.e. presumably __le32 as written today.
> +
> +struct qcom_qps615_pwrctl_ctx {
> + struct i2c_adapter *adapter;
> + struct pci_pwrctl pwrctl;
> + struct device_link *link;
> + struct regulator *vdd;
> +};
> +
> +/* write 32-bit value to 24 bit register */
> +static int qps615_switch_i2c_write(struct qcom_qps615_pwrctl_ctx *ctx, u32 slv_addr, u32 reg_addr,
> + u32 reg_val)
addr, offset, val?
> +{
> + struct i2c_msg msg;
> + u8 msg_buf[7];
> + int ret;
> +
> + msg.addr = slv_addr;
> + msg.len = 7;
> + msg.flags = 0;
> +
> + /* Big Endian for reg addr */
> + msg_buf[0] = (u8)(reg_addr >> 16);
> + msg_buf[1] = (u8)(reg_addr >> 8);
> + msg_buf[2] = (u8)reg_addr;
> +
> + /* Little Endian for reg val */
> + msg_buf[3] = (u8)(reg_val);
> + msg_buf[4] = (u8)(reg_val >> 8);
> + msg_buf[5] = (u8)(reg_val >> 16);
> + msg_buf[6] = (u8)(reg_val >> 24);
> +
> + msg.buf = msg_buf;
> + ret = i2c_transfer(ctx->adapter, &msg, 1);
Without qps615 being a child device of the adapter, does the kernel have
adequate information to know that the geni I2C driver needs to be
resumed before the qps615 driver, such that __i2c_transfer() will not
bail as __i2c_check_suspended() reports that the bus is still suspended?
> + return ret == 1 ? 0 : ret;
> +}
> +
> +/* read 32 bit value from 24 bit reg addr */
> +static int qps615_switch_i2c_read(struct qcom_qps615_pwrctl_ctx *ctx, u32 slv_addr, u32 reg_addr,
> + u32 *reg_val)
> +{
> + u8 wr_data[3], rd_data[4] = {0};
> + struct i2c_msg msg[2];
> + int ret;
> +
> + msg[0].addr = slv_addr;
> + msg[0].len = 3;
> + msg[0].flags = 0;
> +
> + /* Big Endian for reg addr */
> + wr_data[0] = (u8)(reg_addr >> 16);
> + wr_data[1] = (u8)(reg_addr >> 8);
> + wr_data[2] = (u8)reg_addr;
> +
> + msg[0].buf = wr_data;
> +
> + msg[1].addr = slv_addr;
> + msg[1].len = 4;
> + msg[1].flags = I2C_M_RD;
> +
> + msg[1].buf = rd_data;
> +
> + ret = i2c_transfer(ctx->adapter, &msg[0], 2);
> + if (ret != 2)
> + return ret;
> +
> + *reg_val = (rd_data[3] << 24) | (rd_data[2] << 16) | (rd_data[1] << 8) | rd_data[0];
__le32 rd_data;
msg[1].addr = &rd_data;
msg[1].len = sizeof(rd_data);
*value = le32_to_cpu(rd_data);
Or do I get that backwards?
> +
> + return 0;
> +}
> +
> +static int qcom_qps615_pwrctl_init(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> + struct device *dev = ctx->pwrctl.dev;
> + struct qcom_qps615_pwrctl_i2c_setting *set;
> + const struct firmware *fw;
> + const u8 *pos, *eof;
> + int ret;
> + u32 val;
> +
> + ret = request_firmware(&fw, "qcom/qps615.bin", dev);
Is this the generic qps615 firmware, applicable to all implementations
thereof, or does it contain device-specific calibration or such?
> + if (ret < 0) {
> + dev_err(dev, "firmware loading failed with ret %d\n", ret);
> + return ret;
> + }
> +
> + if (!fw) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + pos = fw->data;
> + eof = fw->data + fw->size;
> +
> + while (pos < (fw->data + fw->size)) {
In the case the firmware is truncated by 1 - 11 bytes this would read
outside the buffer. Also, I presume this was the intended use for the
now unused "eof" variable.
> + set = (struct qcom_qps615_pwrctl_i2c_setting *)pos;
I'd suggest that you treat fw->data as an array of struct
qcom_qps615_pwrctl_i2c_setting elements.
> +
> + ret = qps615_switch_i2c_write(ctx, set->slv_addr, set->reg_addr, set->val);
Is it possible to avoid getting the i2c device address from the
firmware? Does/can this change?
> + if (ret) {
> + dev_err(dev,
> + "I2c write failed for slv addr:%x at addr%x with val %x ret %d\n",
Would be nice to clean this up a bit, there are no non-i2c accesses from
this device, and the words "slv", "with", "val" has low SNR. Also %x
does not communicate the base for many values, use %#x to get the 0x
prefix.
Perhaps:
"failed to write %#x to #x on %#x\n", set->val, set->reg_addr, set->slv_addr
> + set->slv_addr, set->reg_addr, set->val, ret);
> + goto err;
> + }
> +
> + ret = qps615_switch_i2c_read(ctx, set->slv_addr, set->reg_addr, &val);
> + if (ret) {
> + dev_err(dev, "I2c read failed for slv addr:%x at addr%x ret %d\n",
"readback failed at %#x on %#x\n"
> + set->slv_addr, set->reg_addr, ret);
> + goto err;
> + }
> +
> + if (set->val != val) {
> + dev_err(dev,
> + "I2c read's mismatch for slv:%x at addr%x exp%d got%d\n",
"readback verification failed at %#x on %#x (%#x != %#x)\n"
If there's only a single slv_addr value, then please omit that from all
three error cases.
> + set->slv_addr, set->reg_addr, set->val, val);
> + goto err;
> + }
> + pos += sizeof(struct qcom_qps615_pwrctl_i2c_setting);
> + }
> +
> +err:
> + release_firmware(fw);
> +
> + return ret;
> +}
> +
> +static int qcom_qps615_power_on(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> + int ret;
> +
> + ret = regulator_enable(ctx->vdd);
> + if (ret) {
> + dev_err(ctx->pwrctl.dev, "cannot enable vdda regulator\n");
Is it vdd or vdda?
> + return ret;
> + }
> +
> + ret = qcom_qps615_pwrctl_init(ctx);
> + if (ret)
> + regulator_disable(ctx->vdd);
> +
> + return ret;
> +}
> +
> +static int qcom_qps615_power_off(struct qcom_qps615_pwrctl_ctx *ctx)
You don't care for the return value, so replace it with void.
> +{
> + return regulator_disable(ctx->vdd);
> +}
> +
> +static int qcom_qps615_pwrctl_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *node = pdev->dev.of_node;
> + struct qcom_qps615_pwrctl_ctx *ctx;
> + struct device_node *adapter_node;
> + struct pci_host_bridge *bridge;
> + struct i2c_adapter *adapter;
> + struct pci_bus *bus;
> +
> + bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> + if (!bus)
> + return -ENODEV;
> +
> + bridge = pci_find_host_bridge(bus);
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + adapter_node = of_parse_phandle(node, "switch-i2c-cntrl", 0);
> + if (adapter_node) {
> + adapter = of_get_i2c_adapter_by_node(adapter_node);
Per of_get_i2c_adapter_by_node() kernel-doc:
"The user must call i2c_put_adapter(adapter) once done with the i2c
adapter."
> + __free(adapter_node);
> + if (!adapter)
> + return dev_err_probe(dev, -EPROBE_DEFER,
> + "failed to parse switch-i2c-cntrl\n");
> + }
> +
> + ctx->pwrctl.dev = dev;
> + ctx->adapter = adapter;
> + ctx->vdd = devm_regulator_get(dev, "vdd");
> + if (IS_ERR(ctx->vdd))
> + return dev_err_probe(dev, PTR_ERR(ctx->vdd),
> + "failed to get vdd regulator\n");
> +
> + ctx->link = device_link_add(&bridge->dev, dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> +
> + platform_set_drvdata(pdev, ctx);
> +
> + bridge->ops->stop_link(bus);
> + qcom_qps615_power_on(ctx);
> + bridge->ops->start_link(bus);
> +
> + return devm_pci_pwrctl_device_set_ready(dev, &ctx->pwrctl);
> +}
> +
> +static const struct of_device_id qcom_qps615_pwrctl_of_match[] = {
> + {
> + .compatible = "pci1179,0623",
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_qps615_pwrctl_of_match);
This would fit better down by qcom_qps615_pwrctl_driver, instead of
mixed in among the implementation.
> +
> +static int pci_pwrctl_suspend_noirq(struct device *dev)
> +{
> + struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> + struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
How about caching the bus and bridge in ctx?
> + struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> + bridge->ops->stop_link(bus);
> + qcom_qps615_power_off(ctx);
> +
> + return 0;
> +}
> +
> +static int pci_pwrctl_resume_noirq(struct device *dev)
> +{
> + struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> + struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> + struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> + qcom_qps615_power_on(ctx);
Perhaps worth checking this return value? Although I guess there's not
much to do if it's non-zero...
Regards,
Bjorn
> + bridge->ops->start_link(bus);
> +
> + return 0;
> +}
> +
> +static void qcom_qps615_pwrctl_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> + device_link_del(ctx->link);
> + pci_pwrctl_suspend_noirq(dev);
> +}
> +
> +static const struct dev_pm_ops pci_pwrctl_pm_ops = {
> + NOIRQ_SYSTEM_SLEEP_PM_OPS(pci_pwrctl_suspend_noirq, pci_pwrctl_resume_noirq)
> +};
> +
> +static struct platform_driver qcom_qps615_pwrctl_driver = {
> + .driver = {
> + .name = "pwrctl-qps615",
> + .of_match_table = qcom_qps615_pwrctl_of_match,
> + .pm = &pci_pwrctl_pm_ops,
> + },
> + .probe = qcom_qps615_pwrctl_probe,
> + .remove_new = qcom_qps615_pwrctl_remove,
> +};
> +module_platform_driver(qcom_qps615_pwrctl_driver);
> +
> +MODULE_AUTHOR("Krishna chaitanya chundru <quic_krichai@quicinc.com>");
> +MODULE_DESCRIPTION("Qualcomm QPS615 power control driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.42.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615
2024-06-26 12:37 ` [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615 Krishna chaitanya chundru
` (2 preceding siblings ...)
2024-06-26 17:00 ` Bjorn Andersson
@ 2024-06-26 18:09 ` Dmitry Baryshkov
2024-07-01 19:54 ` Bjorn Helgaas
4 siblings, 0 replies; 28+ messages in thread
From: Dmitry Baryshkov @ 2024-06-26 18:09 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han, quic_vbadigan, quic_skananth, quic_nitegupt,
linux-arm-msm, linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 06:07:55PM GMT, Krishna chaitanya chundru wrote:
> QPS615 switch needs to configured after powering on and before
> PCIe link was up.
>
> As the PCIe controller driver already enables the PCIe link training
> at the host side, stop the link training.
> Otherwise the moment we turn on the switch it will participate in
> the link training and link may come before switch is configured through
> i2c.
>
> The switch can be configured different ways like changing de-emphasis
> settings of the switch, disabling unused ports etc and these settings
> can vary from board to board, for that reason the sequence is taken
> from the firmware file which contains the address of the slave, to address
> and data to be written to the switch. The driver reads the firmware file
> and parses them to apply those configurations to the switch.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> drivers/pci/pwrctl/Kconfig | 7 +
> drivers/pci/pwrctl/Makefile | 1 +
> drivers/pci/pwrctl/pci-pwrctl-qps615.c | 278 +++++++++++++++++++++++++++++++++
> 3 files changed, 286 insertions(+)
>
> diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
> index f1b824955d4b..a419b250006d 100644
> --- a/drivers/pci/pwrctl/Kconfig
> +++ b/drivers/pci/pwrctl/Kconfig
> @@ -14,4 +14,11 @@ config PCI_PWRCTL_PWRSEQ
> Enable support for the PCI power control driver for device
> drivers using the Power Sequencing subsystem.
>
> +config PCI_PWRCTL_QPS615
> + tristate "PCI Power Control driver for QPS615"
> + select PCI_PWRCTL
> + help
> + Enable support for the PCI power control driver for QPS615 and
> + configures it.
> +
> endmenu
> diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile
> index d308aae4800c..ac563a70c023 100644
> --- a/drivers/pci/pwrctl/Makefile
> +++ b/drivers/pci/pwrctl/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_PCI_PWRCTL) += pci-pwrctl-core.o
> pci-pwrctl-core-y := core.o
>
> obj-$(CONFIG_PCI_PWRCTL_PWRSEQ) += pci-pwrctl-pwrseq.o
> +obj-$(CONFIG_PCI_PWRCTL_QPS615) += pci-pwrctl-qps615.o
> diff --git a/drivers/pci/pwrctl/pci-pwrctl-qps615.c b/drivers/pci/pwrctl/pci-pwrctl-qps615.c
> new file mode 100644
> index 000000000000..1f2caf5d7da2
> --- /dev/null
> +++ b/drivers/pci/pwrctl/pci-pwrctl-qps615.c
> @@ -0,0 +1,278 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/firmware.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pci-pwrctl.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +
> +#include "../pci.h"
> +
> +struct qcom_qps615_pwrctl_i2c_setting {
> + u32 slv_addr;
> + u32 reg_addr;
> + u32 val;
> +};
> +
> +struct qcom_qps615_pwrctl_ctx {
> + struct i2c_adapter *adapter;
> + struct pci_pwrctl pwrctl;
> + struct device_link *link;
> + struct regulator *vdd;
> +};
> +
> +/* write 32-bit value to 24 bit register */
> +static int qps615_switch_i2c_write(struct qcom_qps615_pwrctl_ctx *ctx, u32 slv_addr, u32 reg_addr,
> + u32 reg_val)
> +{
> + struct i2c_msg msg;
> + u8 msg_buf[7];
> + int ret;
> +
> + msg.addr = slv_addr;
> + msg.len = 7;
> + msg.flags = 0;
> +
> + /* Big Endian for reg addr */
> + msg_buf[0] = (u8)(reg_addr >> 16);
> + msg_buf[1] = (u8)(reg_addr >> 8);
> + msg_buf[2] = (u8)reg_addr;
> +
> + /* Little Endian for reg val */
> + msg_buf[3] = (u8)(reg_val);
> + msg_buf[4] = (u8)(reg_val >> 8);
> + msg_buf[5] = (u8)(reg_val >> 16);
> + msg_buf[6] = (u8)(reg_val >> 24);
> +
> + msg.buf = msg_buf;
> + ret = i2c_transfer(ctx->adapter, &msg, 1);
> + return ret == 1 ? 0 : ret;
> +}
> +
> +/* read 32 bit value from 24 bit reg addr */
> +static int qps615_switch_i2c_read(struct qcom_qps615_pwrctl_ctx *ctx, u32 slv_addr, u32 reg_addr,
> + u32 *reg_val)
> +{
> + u8 wr_data[3], rd_data[4] = {0};
> + struct i2c_msg msg[2];
> + int ret;
> +
> + msg[0].addr = slv_addr;
> + msg[0].len = 3;
> + msg[0].flags = 0;
> +
> + /* Big Endian for reg addr */
> + wr_data[0] = (u8)(reg_addr >> 16);
> + wr_data[1] = (u8)(reg_addr >> 8);
> + wr_data[2] = (u8)reg_addr;
> +
> + msg[0].buf = wr_data;
> +
> + msg[1].addr = slv_addr;
> + msg[1].len = 4;
> + msg[1].flags = I2C_M_RD;
> +
> + msg[1].buf = rd_data;
> +
> + ret = i2c_transfer(ctx->adapter, &msg[0], 2);
> + if (ret != 2)
> + return ret;
> +
> + *reg_val = (rd_data[3] << 24) | (rd_data[2] << 16) | (rd_data[1] << 8) | rd_data[0];
> +
> + return 0;
> +}
> +
> +static int qcom_qps615_pwrctl_init(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> + struct device *dev = ctx->pwrctl.dev;
> + struct qcom_qps615_pwrctl_i2c_setting *set;
> + const struct firmware *fw;
> + const u8 *pos, *eof;
> + int ret;
> + u32 val;
> +
> + ret = request_firmware(&fw, "qcom/qps615.bin", dev);
Oh, what if any other board uses the same bridge? Do you expect that the
rootfs is device-specific?
> + if (ret < 0) {
> + dev_err(dev, "firmware loading failed with ret %d\n", ret);
> + return ret;
> + }
> +
> + if (!fw) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + pos = fw->data;
> + eof = fw->data + fw->size;
> +
> + while (pos < (fw->data + fw->size)) {
> + set = (struct qcom_qps615_pwrctl_i2c_setting *)pos;
> +
> + ret = qps615_switch_i2c_write(ctx, set->slv_addr, set->reg_addr, set->val);
No! This makes no way for anybody to understand what is going on. Please
write a proper driver, that defines registers and writes sensible data
to those registers. Having a driver that writes data from the
configuration file to the random registers is a no-go.
> + if (ret) {
> + dev_err(dev,
> + "I2c write failed for slv addr:%x at addr%x with val %x ret %d\n",
> + set->slv_addr, set->reg_addr, set->val, ret);
> + goto err;
> + }
> +
> + ret = qps615_switch_i2c_read(ctx, set->slv_addr, set->reg_addr, &val);
> + if (ret) {
> + dev_err(dev, "I2c read failed for slv addr:%x at addr%x ret %d\n",
> + set->slv_addr, set->reg_addr, ret);
> + goto err;
> + }
> +
> + if (set->val != val) {
> + dev_err(dev,
> + "I2c read's mismatch for slv:%x at addr%x exp%d got%d\n",
> + set->slv_addr, set->reg_addr, set->val, val);
> + goto err;
> + }
> + pos += sizeof(struct qcom_qps615_pwrctl_i2c_setting);
> + }
> +
> +err:
> + release_firmware(fw);
> +
> + return ret;
> +}
> +
> +static int qcom_qps615_power_on(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> + int ret;
> +
> + ret = regulator_enable(ctx->vdd);
> + if (ret) {
> + dev_err(ctx->pwrctl.dev, "cannot enable vdda regulator\n");
> + return ret;
> + }
> +
> + ret = qcom_qps615_pwrctl_init(ctx);
> + if (ret)
> + regulator_disable(ctx->vdd);
> +
> + return ret;
> +}
> +
> +static int qcom_qps615_power_off(struct qcom_qps615_pwrctl_ctx *ctx)
> +{
> + return regulator_disable(ctx->vdd);
> +}
> +
> +static int qcom_qps615_pwrctl_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *node = pdev->dev.of_node;
> + struct qcom_qps615_pwrctl_ctx *ctx;
> + struct device_node *adapter_node;
> + struct pci_host_bridge *bridge;
> + struct i2c_adapter *adapter;
> + struct pci_bus *bus;
> +
> + bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> + if (!bus)
> + return -ENODEV;
> +
> + bridge = pci_find_host_bridge(bus);
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + adapter_node = of_parse_phandle(node, "switch-i2c-cntrl", 0);
> + if (adapter_node) {
> + adapter = of_get_i2c_adapter_by_node(adapter_node);
Somebody didn't read the comment before of_get_i2c_adapter_by_node().
> + __free(adapter_node);
> + if (!adapter)
> + return dev_err_probe(dev, -EPROBE_DEFER,
> + "failed to parse switch-i2c-cntrl\n");
> + }
> +
> + ctx->pwrctl.dev = dev;
> + ctx->adapter = adapter;
> + ctx->vdd = devm_regulator_get(dev, "vdd");
> + if (IS_ERR(ctx->vdd))
> + return dev_err_probe(dev, PTR_ERR(ctx->vdd),
> + "failed to get vdd regulator\n");
> +
> + ctx->link = device_link_add(&bridge->dev, dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> +
> + platform_set_drvdata(pdev, ctx);
> +
> + bridge->ops->stop_link(bus);
> + qcom_qps615_power_on(ctx);
> + bridge->ops->start_link(bus);
> +
> + return devm_pci_pwrctl_device_set_ready(dev, &ctx->pwrctl);
> +}
I'd suggest turning this into a driver which uses components to bind the
PCIe power control part and the i2c clinet device. Then you can write
the i2c client addresses as ussual.
> +
> +static const struct of_device_id qcom_qps615_pwrctl_of_match[] = {
> + {
> + .compatible = "pci1179,0623",
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, qcom_qps615_pwrctl_of_match);
> +
> +static int pci_pwrctl_suspend_noirq(struct device *dev)
> +{
> + struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> + struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> + struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> + bridge->ops->stop_link(bus);
> + qcom_qps615_power_off(ctx);
> +
> + return 0;
> +}
> +
> +static int pci_pwrctl_resume_noirq(struct device *dev)
> +{
> + struct pci_bus *bus = pci_find_bus(of_get_pci_domain_nr(dev->parent->of_node), 0);
> + struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> + struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> + qcom_qps615_power_on(ctx);
> + bridge->ops->start_link(bus);
> +
> + return 0;
> +}
> +
> +static void qcom_qps615_pwrctl_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct qcom_qps615_pwrctl_ctx *ctx = dev_get_drvdata(dev);
> +
> + device_link_del(ctx->link);
> + pci_pwrctl_suspend_noirq(dev);
> +}
> +
> +static const struct dev_pm_ops pci_pwrctl_pm_ops = {
> + NOIRQ_SYSTEM_SLEEP_PM_OPS(pci_pwrctl_suspend_noirq, pci_pwrctl_resume_noirq)
> +};
> +
> +static struct platform_driver qcom_qps615_pwrctl_driver = {
> + .driver = {
> + .name = "pwrctl-qps615",
> + .of_match_table = qcom_qps615_pwrctl_of_match,
> + .pm = &pci_pwrctl_pm_ops,
> + },
> + .probe = qcom_qps615_pwrctl_probe,
> + .remove_new = qcom_qps615_pwrctl_remove,
> +};
> +module_platform_driver(qcom_qps615_pwrctl_driver);
> +
> +MODULE_AUTHOR("Krishna chaitanya chundru <quic_krichai@quicinc.com>");
> +MODULE_DESCRIPTION("Qualcomm QPS615 power control driver");
> +MODULE_LICENSE("GPL");
>
> --
> 2.42.0
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615
2024-06-26 12:37 ` [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615 Krishna chaitanya chundru
` (3 preceding siblings ...)
2024-06-26 18:09 ` Dmitry Baryshkov
@ 2024-07-01 19:54 ` Bjorn Helgaas
4 siblings, 0 replies; 28+ messages in thread
From: Bjorn Helgaas @ 2024-07-01 19:54 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han, quic_vbadigan, quic_skananth, quic_nitegupt,
linux-arm-msm, linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 06:07:55PM +0530, Krishna chaitanya chundru wrote:
> QPS615 switch needs to configured after powering on and before
> PCIe link was up.
s/need to configured/needs to be configured/
> As the PCIe controller driver already enables the PCIe link training
> at the host side, stop the link training.
Add blank line between paragraphs or rewrap into a single paragraph.
> Otherwise the moment we turn on the switch it will participate in
> the link training and link may come before switch is configured through
> i2c.
s/link may come before/link may come up before/ ?
> The switch can be configured different ways like changing de-emphasis
> settings of the switch, disabling unused ports etc and these settings
> can vary from board to board, for that reason the sequence is taken
> from the firmware file which contains the address of the slave, to address
> and data to be written to the switch. The driver reads the firmware file
> and parses them to apply those configurations to the switch.
s/to address and data/the address and the data/ ?
s/and parses them/and parses it/ (I assume you parse the file?)
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch
2024-06-26 12:37 [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Krishna chaitanya chundru
` (6 preceding siblings ...)
2024-06-26 12:37 ` [PATCH RFC 7/7] pci: pwrctl: Add power control driver for qps615 Krishna chaitanya chundru
@ 2024-06-26 13:31 ` Rob Herring (Arm)
2024-06-26 15:29 ` Bjorn Helgaas
2024-07-01 17:51 ` Bjorn Helgaas
9 siblings, 0 replies; 28+ messages in thread
From: Rob Herring (Arm) @ 2024-06-26 13:31 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Conor Dooley, linux-kernel, Jingoo Han, linux-arm-msm,
quic_vbadigan, Bjorn Helgaas, linux-pci, Manivannan Sadhasivam,
quic_skananth, devicetree, Bartosz Golaszewski,
Krzysztof Wilczyński, Bjorn Andersson, Konrad Dybcio,
Lorenzo Pieralisi, quic_nitegupt, Krzysztof Kozlowski
On Wed, 26 Jun 2024 18:07:48 +0530, Krishna chaitanya chundru wrote:
> QPS615 is the PCIe switch which has one upstream and three downstream
> ports. One of the downstream ports is used as endpoint device of Ethernet
> MAC. Other two downstream ports are supposed to connect to external
> device. One Host can connect to QPS615 by upstream port.
>
> QPS615 switch power is controlled by the GPIO's. After powering on
> the switch will immediately participate in the link training. if the
> host is also ready by that time PCIe link will established.
>
> The QPS615 needs to configured certain parameters like de-emphasis,
> disable unused port etc before link is established. These settings
> vary from platform to platform.
>
> As the controller starts link training before the probe of pwrctl driver,
> the PCIe link may come up before configuring the switch itself.
> To avoid this introduce two functions in pci_ops to start_link() &
> stop_link() which will disable the link training if the PCIe link is
> not up yet.
>
> Now PCI pwrctl device is the child of the pci-pcie bridge, if we want
> to enable the suspend resume for pwrctl device there may be issues
> since pci bridge will try to access some registers in the config which
> may cause timeouts or Un clocked access as the power can be removed in
> the suspend of pwrctl driver.
>
> To solve this make PCIe controller as parent to the pci pwr ctrl driver
> and create devlink between host bridge and pci pwrctl driver so that
> pci pwrctl driver will go suspend only after all the PCIe devices went
> to suspend.
>
> In pci pwrctl driver use stop_link() to keep the link in D3cold and
> start_link() to bring back link to D0.
>
> This series is developed on top the series:
> https://lore.kernel.org/lkml/20240612082019.19161-1-brgl@bgdev.pl/
>
> we are sending this series to get coments on the usage of stop_link
> and start_link which is being add in this series.
>
> Signed-off-by: Krishna chaitanya chundru <quic_krichai@quicinc.com>
> ---
> Krishna chaitanya chundru (7):
> dt: bindings: add qcom,qps615.yaml
> arm64: dts: qcom: qcs6490-rb3gen2: Add qps615 node
> pci: Change the parent of the platform devices for child OF nodes
> pci: Add new start_link() & stop_link function ops
> pci: dwc: Add support for new pci function op
> pci: qcom: Add support for start_link() & stop_link()
> pci: pwrctl: Add power control driver for qps615
>
> .../devicetree/bindings/pci/qcom,qps615.yaml | 73 ++++++
> arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts | 55 ++++
> drivers/pci/bus.c | 5 +-
> drivers/pci/controller/dwc/pcie-designware-host.c | 19 ++
> drivers/pci/controller/dwc/pcie-qcom.c | 108 +++++++-
> drivers/pci/pwrctl/Kconfig | 7 +
> drivers/pci/pwrctl/Makefile | 1 +
> drivers/pci/pwrctl/core.c | 7 +-
> drivers/pci/pwrctl/pci-pwrctl-qps615.c | 278 +++++++++++++++++++++
> include/linux/pci.h | 2 +
> 10 files changed, 541 insertions(+), 14 deletions(-)
> ---
> base-commit: d737627471e5b3962eedae870aa0475d6c9bba18
> change-id: 20240624-qps615-faa0cc60dc74
>
> Best regards,
> --
> Krishna chaitanya chundru <quic_krichai@quicinc.com>
>
>
>
My bot found new DTB warnings on the .dts files added or changed in this
series.
Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not. No need to reply
unless the platform maintainer has comments.
If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:
pip3 install dtschema --upgrade
New warnings running 'make CHECK_DTBS=y qcom/qcs6490-rb3gen2.dtb' for 20240626-qps615-v1-0-2ade7bd91e02@quicinc.com:
arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts:562.12-567.5: Warning (pci_device_reg): /soc@0/pcie@1c08000/pcie@0/qps615@0: PCI unit address format error, expected "2,0"
arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dts:560.3-27: Warning (pci_device_bus_num): /soc@0/pcie@1c08000/pcie@0/qps615@0:bus-range: PCI bus number 0 out of range, expected (1 - 255)
arch/arm64/boot/dts/qcom/qcs6490-rb3gen2.dtb: rsc@18200000: 'qps615-0p9-vreg', 'qps615-1p8-vreg', 'qps615-rsex-vreg' do not match any of the regexes: '^regulators(-[0-9])?$', 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/soc/qcom/qcom,rpmh-rsc.yaml#
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch
2024-06-26 12:37 [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Krishna chaitanya chundru
` (7 preceding siblings ...)
2024-06-26 13:31 ` [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Rob Herring (Arm)
@ 2024-06-26 15:29 ` Bjorn Helgaas
2024-07-01 17:51 ` Bjorn Helgaas
9 siblings, 0 replies; 28+ messages in thread
From: Bjorn Helgaas @ 2024-06-26 15:29 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han, quic_vbadigan, quic_skananth, quic_nitegupt,
linux-arm-msm, linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 06:07:48PM +0530, Krishna chaitanya chundru wrote:
> Krishna chaitanya chundru (7):
> dt: bindings: add qcom,qps615.yaml
> arm64: dts: qcom: qcs6490-rb3gen2: Add qps615 node
> pci: Change the parent of the platform devices for child OF nodes
> pci: Add new start_link() & stop_link function ops
> pci: dwc: Add support for new pci function op
> pci: qcom: Add support for start_link() & stop_link()
> pci: pwrctl: Add power control driver for qps615
Take a look at the git history of the files you update and match the
style. s/pci/PCI/, for instance.
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch
2024-06-26 12:37 [PATCH RFC 0/7] PCI: enable Power and configure the QPS615 PCIe switch Krishna chaitanya chundru
` (8 preceding siblings ...)
2024-06-26 15:29 ` Bjorn Helgaas
@ 2024-07-01 17:51 ` Bjorn Helgaas
9 siblings, 0 replies; 28+ messages in thread
From: Bjorn Helgaas @ 2024-07-01 17:51 UTC (permalink / raw)
To: Krishna chaitanya chundru
Cc: Bartosz Golaszewski, Manivannan Sadhasivam, Lorenzo Pieralisi,
Krzysztof Wilczyński, Rob Herring, Bjorn Helgaas,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Konrad Dybcio,
Jingoo Han, quic_vbadigan, quic_skananth, quic_nitegupt,
linux-arm-msm, linux-pci, devicetree, linux-kernel
On Wed, Jun 26, 2024 at 06:07:48PM +0530, Krishna chaitanya chundru wrote:
> dt: bindings: add qcom,qps615.yaml
> arm64: dts: qcom: qcs6490-rb3gen2: Add qps615 node
> pci: Change the parent of the platform devices for child OF nodes
> pci: Add new start_link() & stop_link function ops
> pci: dwc: Add support for new pci function op
> pci: qcom: Add support for start_link() & stop_link()
> pci: pwrctl: Add power control driver for qps615
Run "git log --oneline" on these files and match the style.
"PCI: Change ...", "PCI: qcom:", "PCI/pwrctl", etc.
The colon vs slash convention isn't obvious, but this is how we've
applied it in the past:
For PCI core features like MSI, PM, ASPM, AER, DOE, etc:
PCI/<feature>: <Text>
For PCI drivers outside the PCI core, like dwc, qcom, imx6, etc:
PCI: <driver name>: <Text>
^ permalink raw reply [flat|nested] 28+ messages in thread