| Message ID | 20230318121828.739424-10-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Qualcomm PMIC TPCM support | expand |
On 18/03/2023 13:18, Bryan O'Donoghue wrote: > Add a YAML description for the pm8150b-tcpm driver. The pm8150b-tcpm No, do not add YAML description for driver. Please add bindings for some hardware and describe the hardware. > encapsulates a type-c block and a pdphy block into one block presented to > the TCPM Linux API. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > .../bindings/usb/qcom,pmic-virt-tcpm.yaml | 88 +++++++++++++++++++ > 1 file changed, 88 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-virt-tcpm.yaml > > diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-virt-tcpm.yaml b/Documentation/devicetree/bindings/usb/qcom,pmic-virt-tcpm.yaml > new file mode 100644 > index 0000000000000..576842c8b65b4 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/qcom,pmic-virt-tcpm.yaml > @@ -0,0 +1,88 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/usb/qcom,pmic-virt-tcpm.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: Qualcomm PMIC Virtual TCPM Driver All previous comments apply. > + > +maintainers: > + - Bryan O'Donoghue <bryan.odonoghue@linaro.org> > + > +description: | > + Qualcomm PMIC Virtual Type-C Port Manager Driver > + A virtual device which manages Qualcomm PMIC provided Type-C port and > + Power Delivery in one place. OK, so it looks like bindings for driver, so a no-go. Unless there is such device as "manager", this does not look like hardware description. > + > +properties: > + compatible: > + const: qcom,pmic-virt-tcpm > + > + connector: > + type: object > + $ref: /schemas/connector/usb-connector.yaml# > + unevaluatedProperties: false > + > + port: > + $ref: /schemas/graph.yaml#/properties/port > + description: > + Contains a port which consumes data-role switching messages. > + > + qcom,pmic-typec: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + A phandle to the typec port hardware driver. > + > + qcom,pmic-pdphy: > + $ref: /schemas/types.yaml#/definitions/phandle Having typec and phy as phandles - not children - also suggests this is some software construct, not hardware description. Best regards, Krzysztof
On 19/03/2023 11:58, Krzysztof Kozlowski wrote: >> + >> +maintainers: >> + - Bryan O'Donoghue<bryan.odonoghue@linaro.org> >> + >> +description: | >> + Qualcomm PMIC Virtual Type-C Port Manager Driver >> + A virtual device which manages Qualcomm PMIC provided Type-C port and >> + Power Delivery in one place. > OK, so it looks like bindings for driver, so a no-go. Unless there is > such device as "manager", this does not look like hardware description. > >> + >> +properties: >> + compatible: >> + const: qcom,pmic-virt-tcpm >> + >> + connector: >> + type: object >> + $ref: /schemas/connector/usb-connector.yaml# >> + unevaluatedProperties: false >> + >> + port: >> + $ref: /schemas/graph.yaml#/properties/port >> + description: >> + Contains a port which consumes data-role switching messages. >> + >> + qcom,pmic-typec: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: >> + A phandle to the typec port hardware driver. >> + >> + qcom,pmic-pdphy: >> + $ref: /schemas/types.yaml#/definitions/phandle > Having typec and phy as phandles - not children - also suggests this is > some software construct, not hardware description. So probably I didn't interpret Rob's comment correctly here. For a pure software device - a virtual device - there should be no dts representation at all - not even at the firmware{}, chosen{}, rpm{} level, it wouldn't be possible/acceptable to have a tcpm {} with a compat pointing to the two phandles I have here ? --- bod
On 19/03/2023 15:59, Bryan O'Donoghue wrote: > On 19/03/2023 11:58, Krzysztof Kozlowski wrote: >>> + >>> +maintainers: >>> + - Bryan O'Donoghue<bryan.odonoghue@linaro.org> >>> + >>> +description: | >>> + Qualcomm PMIC Virtual Type-C Port Manager Driver >>> + A virtual device which manages Qualcomm PMIC provided Type-C port and >>> + Power Delivery in one place. >> OK, so it looks like bindings for driver, so a no-go. Unless there is >> such device as "manager", this does not look like hardware description. >> >>> + >>> +properties: >>> + compatible: >>> + const: qcom,pmic-virt-tcpm >>> + >>> + connector: >>> + type: object >>> + $ref: /schemas/connector/usb-connector.yaml# >>> + unevaluatedProperties: false >>> + >>> + port: >>> + $ref: /schemas/graph.yaml#/properties/port >>> + description: >>> + Contains a port which consumes data-role switching messages. >>> + >>> + qcom,pmic-typec: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: >>> + A phandle to the typec port hardware driver. >>> + >>> + qcom,pmic-pdphy: >>> + $ref: /schemas/types.yaml#/definitions/phandle >> Having typec and phy as phandles - not children - also suggests this is >> some software construct, not hardware description. > > So probably I didn't interpret Rob's comment correctly here. He proposed to merge it with other node: "probably merged with one of the nodes these phandles point to." "Why can't most of this binding be part of" I don't see how you implemented his comments. Actually, nothing improved here in this regard - you still have these phandles. > > For a pure software device - a virtual device - there should be no dts > representation at all - not even at the firmware{}, chosen{}, rpm{} By software we interpret here HLOS, so Linux. Firmware is FW, thus not software in that context. rpm{} is some firmware on some processor. You wrote here bindings/nodes for a Linux driver. > level, it wouldn't be possible/acceptable to have a tcpm {} with a > compat pointing to the two phandles I have here ? What is tcpm? Linux driver? Then not. You cannot have device nodes for a Linux driver. Best regards, Krzysztof
On 19/03/2023 15:10, Krzysztof Kozlowski wrote: > On 19/03/2023 15:59, Bryan O'Donoghue wrote: >> On 19/03/2023 11:58, Krzysztof Kozlowski wrote: >>>> + >>>> +maintainers: >>>> + - Bryan O'Donoghue<bryan.odonoghue@linaro.org> >>>> + >>>> +description: | >>>> + Qualcomm PMIC Virtual Type-C Port Manager Driver >>>> + A virtual device which manages Qualcomm PMIC provided Type-C port and >>>> + Power Delivery in one place. >>> OK, so it looks like bindings for driver, so a no-go. Unless there is >>> such device as "manager", this does not look like hardware description. >>> >>>> + >>>> +properties: >>>> + compatible: >>>> + const: qcom,pmic-virt-tcpm >>>> + >>>> + connector: >>>> + type: object >>>> + $ref: /schemas/connector/usb-connector.yaml# >>>> + unevaluatedProperties: false >>>> + >>>> + port: >>>> + $ref: /schemas/graph.yaml#/properties/port >>>> + description: >>>> + Contains a port which consumes data-role switching messages. >>>> + >>>> + qcom,pmic-typec: >>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>> + description: >>>> + A phandle to the typec port hardware driver. >>>> + >>>> + qcom,pmic-pdphy: >>>> + $ref: /schemas/types.yaml#/definitions/phandle >>> Having typec and phy as phandles - not children - also suggests this is >>> some software construct, not hardware description. >> >> So probably I didn't interpret Rob's comment correctly here. > > He proposed to merge it with other node: > "probably merged with > one of the nodes these phandles point to." > > "Why can't most of this binding be part of" > > I don't see how you implemented his comments. Actually, nothing improved > here in this regard - you still have these phandles. So this comment from Rob is what I was aiming for "Your other option is instantiate your own device from the virtual driver's initcall based on presence of the 2 nodes above. " rather than two mush the pdphy and typec into one device, which they are not. I guess what I'm trying to understand is how you guys would suggest that is actually done. Could I trouble you for an example ? --- bod
On 19/03/2023 15:10, Krzysztof Kozlowski wrote: > What is tcpm? Linux driver? Then not. You cannot have device nodes for a > Linux driver. Hmm. Well, actually I'll just - concatonate these into one node but, it will have to be called something like "typec" and encompass both hardware blocks. I'll try to make the name of that make sense. --- bod
On 19/03/2023 16:44, Bryan O'Donoghue wrote: > On 19/03/2023 15:10, Krzysztof Kozlowski wrote: >> On 19/03/2023 15:59, Bryan O'Donoghue wrote: >>> On 19/03/2023 11:58, Krzysztof Kozlowski wrote: >>>>> + >>>>> +maintainers: >>>>> + - Bryan O'Donoghue<bryan.odonoghue@linaro.org> >>>>> + >>>>> +description: | >>>>> + Qualcomm PMIC Virtual Type-C Port Manager Driver >>>>> + A virtual device which manages Qualcomm PMIC provided Type-C port and >>>>> + Power Delivery in one place. >>>> OK, so it looks like bindings for driver, so a no-go. Unless there is >>>> such device as "manager", this does not look like hardware description. >>>> >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + const: qcom,pmic-virt-tcpm >>>>> + >>>>> + connector: >>>>> + type: object >>>>> + $ref: /schemas/connector/usb-connector.yaml# >>>>> + unevaluatedProperties: false >>>>> + >>>>> + port: >>>>> + $ref: /schemas/graph.yaml#/properties/port >>>>> + description: >>>>> + Contains a port which consumes data-role switching messages. >>>>> + >>>>> + qcom,pmic-typec: >>>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>>> + description: >>>>> + A phandle to the typec port hardware driver. >>>>> + >>>>> + qcom,pmic-pdphy: >>>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>> Having typec and phy as phandles - not children - also suggests this is >>>> some software construct, not hardware description. >>> >>> So probably I didn't interpret Rob's comment correctly here. >> >> He proposed to merge it with other node: >> "probably merged with >> one of the nodes these phandles point to." >> >> "Why can't most of this binding be part of" >> >> I don't see how you implemented his comments. Actually, nothing improved >> here in this regard - you still have these phandles. > > So this comment from Rob is what I was aiming for > > "Your other option is instantiate your own device from the virtual > driver's initcall based on presence of the 2 nodes above. " > > rather than two mush the pdphy and typec into one device, which they are > not. Sure, but you did not instantiate anything based on these two or one nodes. You added virtual device node. > I guess what I'm trying to understand is how you guys would suggest that > is actually done. You have there already node for the PMIC USB Type-C, so this should be part of it. I really do not understand why this is separate device lying around in parallel like: pmic { usb { }; }; virtual- pmic-tcpm { }; What hardware piece does such description represent? > > Could I trouble you for an example ? > > --- > bod Best regards, Krzysztof
On 19/03/2023 17:50, Krzysztof Kozlowski wrote: > On 19/03/2023 16:44, Bryan O'Donoghue wrote: >> On 19/03/2023 15:10, Krzysztof Kozlowski wrote: >>> On 19/03/2023 15:59, Bryan O'Donoghue wrote: >>>> On 19/03/2023 11:58, Krzysztof Kozlowski wrote: >>>>>> + >>>>>> +maintainers: >>>>>> + - Bryan O'Donoghue<bryan.odonoghue@linaro.org> >>>>>> + >>>>>> +description: | >>>>>> + Qualcomm PMIC Virtual Type-C Port Manager Driver >>>>>> + A virtual device which manages Qualcomm PMIC provided Type-C port and >>>>>> + Power Delivery in one place. >>>>> OK, so it looks like bindings for driver, so a no-go. Unless there is >>>>> such device as "manager", this does not look like hardware description. >>>>> >>>>>> + >>>>>> +properties: >>>>>> + compatible: >>>>>> + const: qcom,pmic-virt-tcpm >>>>>> + >>>>>> + connector: >>>>>> + type: object >>>>>> + $ref: /schemas/connector/usb-connector.yaml# >>>>>> + unevaluatedProperties: false >>>>>> + >>>>>> + port: >>>>>> + $ref: /schemas/graph.yaml#/properties/port >>>>>> + description: >>>>>> + Contains a port which consumes data-role switching messages. >>>>>> + >>>>>> + qcom,pmic-typec: >>>>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>>>> + description: >>>>>> + A phandle to the typec port hardware driver. >>>>>> + >>>>>> + qcom,pmic-pdphy: >>>>>> + $ref: /schemas/types.yaml#/definitions/phandle >>>>> Having typec and phy as phandles - not children - also suggests this is >>>>> some software construct, not hardware description. >>>> >>>> So probably I didn't interpret Rob's comment correctly here. >>> >>> He proposed to merge it with other node: >>> "probably merged with >>> one of the nodes these phandles point to." >>> >>> "Why can't most of this binding be part of" >>> >>> I don't see how you implemented his comments. Actually, nothing improved >>> here in this regard - you still have these phandles. >> >> So this comment from Rob is what I was aiming for >> >> "Your other option is instantiate your own device from the virtual >> driver's initcall based on presence of the 2 nodes above. " >> >> rather than two mush the pdphy and typec into one device, which they are >> not. > > Sure, but you did not instantiate anything based on these two or one > nodes. You added virtual device node. > > >> I guess what I'm trying to understand is how you guys would suggest that >> is actually done. > > You have there already node for the PMIC USB Type-C, so this should be > part of it. I really do not understand why this is separate device lying > around in parallel like: The pdphy is fairly well encapsulated (3 tcpm callbacks go to it, that's all?), I think the tcpm part could be merged in with the typec driver and it could just have a phandle to the pdphy node to represent the dependency. Then in the typec driver you can get the device with spmi_device_from_of() and call into it that way for the few tcpm callbacks that it needs to handle and to pass in the tcpm_port. > > pmic { > usb { > }; > }; > > virtual- pmic-tcpm { > }; > > What hardware piece does such description represent? > >> >> Could I trouble you for an example ? >> >> --- >> bod > > Best regards, > Krzysztof >
On 19/03/2023 17:50, Krzysztof Kozlowski wrote: >> So this comment from Rob is what I was aiming for >> >> "Your other option is instantiate your own device from the virtual >> driver's initcall based on presence of the 2 nodes above. " >> >> rather than two mush the pdphy and typec into one device, which they are >> not. > Sure, but you did not instantiate anything based on these two or one > nodes. You added virtual device node. Yes true, but I see the distinction you are making. > >> I guess what I'm trying to understand is how you guys would suggest that >> is actually done. > You have there already node for the PMIC USB Type-C, so this should be > part of it. I really do not understand why this is separate device lying > around in parallel like: > > pmic { > usb { > }; > }; > > virtual- pmic-tcpm { > }; > > What hardware piece does such description represent? > None, yes its a "HLOS" convenience, I take your point.
On 19/03/2023 21:31, Caleb Connolly wrote: > The pdphy is fairly well encapsulated (3 tcpm callbacks go to it, that's > all?), I think the tcpm part could be merged in with the typec driver > and it could just have a phandle to the pdphy node to represent the > dependency. > > Then in the typec driver you can get the device with > spmi_device_from_of() and call into it that way for the few tcpm > callbacks that it needs to handle and to pass in the tcpm_port. Yes or just have one "typec" device own both register banks --- bod
diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-virt-tcpm.yaml b/Documentation/devicetree/bindings/usb/qcom,pmic-virt-tcpm.yaml new file mode 100644 index 0000000000000..576842c8b65b4 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/qcom,pmic-virt-tcpm.yaml @@ -0,0 +1,88 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/usb/qcom,pmic-virt-tcpm.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Qualcomm PMIC Virtual TCPM Driver + +maintainers: + - Bryan O'Donoghue <bryan.odonoghue@linaro.org> + +description: | + Qualcomm PMIC Virtual Type-C Port Manager Driver + A virtual device which manages Qualcomm PMIC provided Type-C port and + Power Delivery in one place. + +properties: + compatible: + const: qcom,pmic-virt-tcpm + + connector: + type: object + $ref: /schemas/connector/usb-connector.yaml# + unevaluatedProperties: false + + port: + $ref: /schemas/graph.yaml#/properties/port + description: + Contains a port which consumes data-role switching messages. + + qcom,pmic-typec: + $ref: /schemas/types.yaml#/definitions/phandle + description: + A phandle to the typec port hardware driver. + + qcom,pmic-pdphy: + $ref: /schemas/types.yaml#/definitions/phandle + description: + A phandle to the type-c pdphy hardware driver. + +required: + - compatible + - connector + - port + - qcom,pmic-typec + - qcom,pmic-pdphy + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/usb/pd.h> + #include <dt-bindings/usb/typec/qcom,pmic-typec.h> + #include <dt-bindings/usb/typec/qcom,pmic-pdphy.h> + + pm8150b_tcpm: pmic-tcpm { + compatible = "qcom,pmic-virt-tcpm"; + + qcom,pmic-typec = <&pm8150b_typec>; + qcom,pmic-pdphy = <&pm8150b_pdphy>; + + port { + usb3_role: endpoint { + remote-endpoint = <&dwc3_drd_switch>; + }; + }; + + connector { + compatible = "usb-c-connector"; + + power-role = "source"; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + pmic_tcpm_ss_mux: endpoint { + remote-endpoint = <&qmp_ss_mux>; + }; + }; + }; + }; + }; + +...
Add a YAML description for the pm8150b-tcpm driver. The pm8150b-tcpm encapsulates a type-c block and a pdphy block into one block presented to the TCPM Linux API. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- .../bindings/usb/qcom,pmic-virt-tcpm.yaml | 88 +++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-virt-tcpm.yaml