RE: [PATCH v2 10/12] dt-bindings: connector: add properties for typec power delivery

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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�����٥




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux