Hi > -----Original Message----- > From: Andrzej Hajda [mailto:a.hajda@xxxxxxxxxxx] > Sent: 2018年3月5日 17:59 > To: Jun Li <jun.li@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; > robh+dt@xxxxxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx; linux@xxxxxxxxxxxx > Cc: mark.rutland@xxxxxxx; yueyao@xxxxxxxxxx; Peter Chen > <peter.chen@xxxxxxx>; garsilva@xxxxxxxxxxxxxx; o_leveque@xxxxxxxxx; > shufan_lee@xxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > Subject: Re: [PATCH v2 10/12] dt-bindings: connector: add properties for > typec power delivery > > On 05.03.2018 08:00, Jun Li wrote: > > > >> -----Original Message----- > >> From: Andrzej Hajda [mailto:a.hajda@xxxxxxxxxxx] > >> Sent: 2018年2月27日 16:41 > >> To: Jun Li <jun.li@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; > >> robh+dt@xxxxxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx; > >> robh+linux@xxxxxxxxxxxx > >> Cc: mark.rutland@xxxxxxx; yueyao@xxxxxxxxxx; Peter Chen > >> <peter.chen@xxxxxxx>; garsilva@xxxxxxxxxxxxxx; > o_leveque@xxxxxxxxx; > >> shufan_lee@xxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; > >> devicetree@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx> > >> Subject: Re: [PATCH v2 10/12] dt-bindings: connector: add properties > >> for typec power delivery > >> > >> On 26.02.2018 12:49, Li Jun wrote: > >>> In case of usb-c-connector with power delivery support, add > >>> bingdings supported by current typec driver, so user can pass all > >>> those properties via dt. > >>> > >>> Signed-off-by: Li Jun <jun.li@xxxxxxx> > >>> --- > >>> Changes for v2: > >>> - Added typec properties are based on general usb connector bindings[1] > >>> proposed by Andrzej Hajda. > >>> - Use the standard unit suffixes as defined in property-units.txt. > >>> > >>> [1] > >>> > >> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpa > >> t > >> > chwork.kernel.org%2Fpatch%2F10231447%2F&data=02%7C01%7Cjun.li%40 > >> nxp.co > >> > m%7Cccf14a36ca6445ee5f1108d57dbde3c7%7C686ea1d3bc2b4c6fa92cd99 > >> c5c30163 > >> > 5%7C0%7C0%7C636553176880292197&sdata=2Pi0AtiwAqHQE3rGl%2Bo49K > >> 7yZZcp%2B > >>> 5bvJAknRBMGTrk%3D&reserved=0 > >>> > >>> .../bindings/connector/usb-connector.txt | 43 > >> ++++++++++++++++++++++ > >>> 1 file changed, 43 insertions(+) > >>> > >>> diff --git > >>> a/Documentation/devicetree/bindings/connector/usb-connector.txt > >>> b/Documentation/devicetree/bindings/connector/usb-connector.txt > >>> index e1463f1..242f6df 100644 > >>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt > >>> +++ > b/Documentation/devicetree/bindings/connector/usb-connector.txt > >>> @@ -15,6 +15,30 @@ Optional properties: > >>> - type: size of the connector, should be specified in case of USB-A, > USB-B > >>> non-fullsize connectors: "mini", "micro". > >>> > >>> +Required properties for usb-c-connector with power delivery support: > >>> +- port-type: should be one of "source", "sink" or "dual". > >>> +- default-role: preferred power role if port-type is "dual"(drp), > >>> +should be > >>> + "sink" or "source". > >> Since port carries data and power, it would be better to explicitly > >> mention it in names of properties which can be ambiguous. > >> Maybe instead of 'port-type' you can use 'power-role', for example. > > Port-type is align with the name of typec coding and ABI, 'power-role' > > is more like for the current role rather than the port's ability. > > I am not sure what are you exactly mean by "coding and ABI", but I guess it is > just about Power-Delivery part of USB-C. And if you want to put this property > into USB node without mark it is related to PD part of USB connector, you > risk confusion with possible USB data related properties. Understood your concern, I am following typec naming conventions, for typec, port-type property has the same meaning as /sys/class/typec/portx/port_type which is not only for power, also for data, there are dedicated sys files power_role for power and data_role for data. > Simple question, what if somebody wants to add property describing USB > data capabilities (DFP, UFP, DRD), how should it be named? > Then we may use data-role? > > > >> Other thing is that default-role is required only in case of DRP, so > >> maybe better would be to squash it in power-role, for example: > >> power-role: should be on of "source", "sink", "dual-source", > >> "dual-sink", in case of dual roles suffix determines preferred role. > > I don't know how much this squash can benefit, "dual-source/sink" is > > not directly showing its meaning by name. > > Some benefit is simpler binding, no conditionally-required property. > There is already string definition for port type and preferred role parse static const char * const typec_port_types[] = { [TYPEC_PORT_DFP] = "source", [TYPEC_PORT_UFP] = "sink", [TYPEC_PORT_DRP] = "dual", }; And I think there will be other conditionally-required properties to be extended later, are we going to name all of them by this way? Either way is OK for me, I am not sure if there is principle like we should avoid conditionally-required property, if we should, maybe "dual-preferred-source/sink" is better. Hi Heikki, Do you have any comments/preference here? > Another question is that USB TypeC specification describes 7 different > "behavioral models" [1]: > - Source-only Maps "source" > - Source (Default) – strong preference toward being a Source but > subsequently capable of becoming a Sink using USB PD swap mechanisms. Only present Rp(source) when attach, can be sink only by power swap. Seems current code doesn't support this, to be extended. > - Sink-only Maps to "sink" > - Sink (Default) – strong preference toward being a Sink but subsequently > capable of becoming a Source using USB PD swap mechanisms. Only present Rd while attachment, can be source only by power swap support. Seems current code doesn't support this, to be extended. > - DRP: Toggling (Source/Sink) Maps to "dual" > - DRP: Sourcing Device "dual" but without USB host function(to be extended). > - DRP: Sinking Host > "dual" but without USB device function(to be extended). > How it maps to your proposed props? > > [1]: USB Type-C specification release 1.3, chapter 4.5.1.4. > Current typec and tcpm hasn't cover the full features like this. Thanks Jun Li > Regards > Andrzej > > > > >> > >>> +- src-pdos: An array of u32 with each entry providing supported > >>> +power > >>> + source data object(PDO), the detailed bit definitions of PDO can > >>> +be found > >>> + in "Universal Serial Bus Power Delivery Specification" chapter > >>> +6.4.1.2 > >>> + Source_Capabilities Message, the order of each entry(PDO) should > >>> +follow > >>> + the PD spec chapter 6.4.1. Required for power source and power > >>> +dual > >> role. > >>> +- snk-pdos: An array of u32 with each entry providing supported > >>> +power > >>> + sink data object(PDO), the detailed bit definitions of PDO can be > >>> +found in > >>> + "Universal Serial Bus Power Delivery Specification" chapter > >>> +6.4.1.3 Sink > >>> + Capabilities Message, the order of each entry(PDO) should follow > >>> +the PD > >>> + spec chapter 6.4.1. Required for power sink and power dual role. > >> We should avoid magic numbers, magic numbers are bad :) If we really > >> need to use PDOs here, I think we can re-use PDO_* macros [1] - DTC > >> should be able to parse it, maybe some lifting will be necessary. > >> > >> [1]: > >> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fel > >> ix > >> > ir.bootlin.com%2Flinux%2Fv4.16-rc3%2Fsource%2Finclude%2Flinux%2Fusb% > >> > 2Fpd.h%23L161&data=02%7C01%7Cjun.li%40nxp.com%7Cccf14a36ca6445e > >> > e5f1108d57dbde3c7%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0% > >> > 7C636553176880292197&sdata=72HA33wgRyd2PMoPnZOlMatI2CuadplkYN > >> DDGKFSUB0%3D&reserved=0 > >>> +- max-snk-microvolt: The max voltage the sink can support in micro > >>> +volts, > >>> + required for power sink and power dual role. > >>> +- max-snk-microamp: The max current the sink can support in micro > >>> +amps, > >>> + required for power sink and power dual role. > >>> +- max-snk-microwatt-hours: The max power the sink can support in > >>> +micro > >>> + Watt-hours, required for power sink and power dual role. > >>> +- op-snk-microwatt-hours: Sink required operating power in micro > >>> +Watt-hours, > >>> + if source offered power is less then it, Capability Mismatch is > >>> +set, > >>> + required for power sink and power dual role. > >> What is the relation between above properties and PDOs specified > earlier? > >> Are you sure all these props are required? > >> > > Sorry, with latest tcpm code, those props are not required, will remove > them. > > > > Jun Li > >> And general remark regarding all above properties. For me, most of > >> them are specific not to the port but to devices responsible for > >> power > >> management: chargers, pmics,.... > >> In many cases these properties are redundant with devices capabilities. > >> On the other side I guess in many cases it may be convenient to put > >> them here, so I am not sure what is better :) > >> > >> Regards > >> Andrzej > >> > >>> + > >>> Required nodes: > >>> - any data bus to the connector should be modeled using the OF > >>> graph > >> bindings > >>> specified in bindings/graph.txt, unless the bus is between parent > >>> node and @@ -73,3 +97,22 @@ ccic: s2mm005@33 { > >>> }; > >>> }; > >>> }; > >>> + > >>> +3. USB-C connector attached to a typec port controller(ptn5110), > >>> +which has power delivery support and enables drp. > >>> + > >>> +typec: ptn5110@50 { > >>> + ... > >>> + usb_con: connector { > >>> + compatible = "usb-c-connector"; > >>> + label = "USB-C"; > >>> + port-type = "dual"; > >>> + default-role = "sink"; > >>> + src-pdos = <0x380190c8>; > >>> + snk-pdos = <0x380190c8 0x3802d0c8>; > >>> + max-snk-microvolt = <9000>; > >>> + max-snk-microamp = <2000>; > >>> + max-snk-microwatt-hours = <18000>; > >>> + op-snk-microwatt-hours = <9000>; > >>> + }; > >>> +}; > ��.n��������+%������w��{.n�����{���)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
