diff mbox

[v5,1/6] dt-bindings: add bindings for USB physical connector

Message ID 20180227071134.28063-2-a.hajda@samsung.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Andrzej Hajda Feb. 27, 2018, 7:11 a.m. UTC
These bindings allow to describe most known standard USB connectors
and it should be possible to extend it if necessary.
USB connectors, beside USB can be used to route other protocols,
for example UART, Audio, MHL. In such case every device passing data
through the connector should have appropriate graph bindings.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
v4:
- improved 'type' description (Rob),
- improved description of 2nd example (Rob).
v3:
- removed MHL port (samsung connector will have separate bindings),
- added 2nd example for USB-C,
- improved formatting.
v2:
- moved connector type(A,B,C) to compatible string (Rob),
- renamed size property to type (Rob),
- changed type description to be less confusing (Laurent),
- removed vendor specific compatibles (implied by graph port number),
- added requirement of connector being a child of IC (Rob),
- removed max-mode (subtly suggested by Rob, it should be detected anyway
  by USB Controller in runtime, downside is that device is not able to
  report its real capabilities, maybe better would be to make it optional(?)),
- assigned port numbers to data buses (Rob).

Regards
Andrzej
---
 .../bindings/connector/usb-connector.txt           | 75 ++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt

Comments

Heikki Krogerus March 2, 2018, 1:13 p.m. UTC | #1
Hi,

On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
> +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
> +
> +ccic: s2mm005@33 {
> +	...
> +	usb_con: connector {
> +		compatible = "usb-c-connector";
> +		label = "USB-C";

Is this child node really necessary? There will never be more then
one connector per CC line.

We should prefer device_graph* functions over of_graph* and
acpi_graph* functions in the drivers so we don't have to handle the
same thing multiple times with separate APIs. Is it still possible if
there is that connector child node?

> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				usb_con_hs: endpoint {
> +					remote-endpoint = <&max77865_usbc_hs>;
> +				};
> +			};
> +			port@1 {
> +				reg = <1>;
> +				usb_con_ss: endpoint {
> +					remote-endpoint = <&usbdrd_phy_ss>;
> +				};
> +			};
> +			port@2 {
> +				reg = <2>;
> +				usb_con_sbu: endpoint {
> +					remote-endpoint = <&dp_aux>;
> +				};
> +			};
> +		};
> +	};
> +};


Thanks,
Andrzej Hajda March 5, 2018, 8:18 a.m. UTC | #2
On 02.03.2018 14:13, Heikki Krogerus wrote:
> Hi,
>
> On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
>> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
>> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
>> +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
>> +
>> +ccic: s2mm005@33 {
>> +	...
>> +	usb_con: connector {
>> +		compatible = "usb-c-connector";
>> +		label = "USB-C";
> Is this child node really necessary? There will never be more then
> one connector per CC line.

But there can be more connectors/cc-lines per IC, for example EZ-PD CCG5[1].

[1]:
http://www.cypress.com/products/ez-pd-ccg5-two-port-usb-type-c-and-power-delivery

>
> We should prefer device_graph* functions over of_graph* and
I guess you mean fwnode_graph* functions.
> acpi_graph* functions in the drivers so we don't have to handle the
> same thing multiple times with separate APIs. Is it still possible if
> there is that connector child node?

Bindings proposed here are OF bindings, I suppose the most important is
to follow OF specification and guidelines and these bindings tries to
follow it.
It looks like it should not be a problem for fwnode framework to handle
such bindings, but it is just my guess. I have not seen any fwnode*
specification I am not sure what is the real purpose of this framework,
but it seems to be just in-kernel abstraction for different firmware
standards (OF, ACPI), so even if it lacks at the moment some
functionality it should not be a barrier for OF bindings.

Regards
Andrzej

>
>> +		ports {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			port@0 {
>> +				reg = <0>;
>> +				usb_con_hs: endpoint {
>> +					remote-endpoint = <&max77865_usbc_hs>;
>> +				};
>> +			};
>> +			port@1 {
>> +				reg = <1>;
>> +				usb_con_ss: endpoint {
>> +					remote-endpoint = <&usbdrd_phy_ss>;
>> +				};
>> +			};
>> +			port@2 {
>> +				reg = <2>;
>> +				usb_con_sbu: endpoint {
>> +					remote-endpoint = <&dp_aux>;
>> +				};
>> +			};
>> +		};
>> +	};
>> +};
>
> Thanks,
>

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Heikki Krogerus March 5, 2018, 10:27 a.m. UTC | #3
On Mon, Mar 05, 2018 at 09:18:10AM +0100, Andrzej Hajda wrote:
> On 02.03.2018 14:13, Heikki Krogerus wrote:
> > Hi,
> >
> > On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
> >> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
> >> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
> >> +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
> >> +
> >> +ccic: s2mm005@33 {
> >> +	...
> >> +	usb_con: connector {
> >> +		compatible = "usb-c-connector";
> >> +		label = "USB-C";
> > Is this child node really necessary? There will never be more then
> > one connector per CC line.
> 
> But there can be more connectors/cc-lines per IC, for example EZ-PD CCG5[1].

OK, in that case the child node is of course needed.

> [1]:
> http://www.cypress.com/products/ez-pd-ccg5-two-port-usb-type-c-and-power-delivery
> 
> >
> > We should prefer device_graph* functions over of_graph* and
> I guess you mean fwnode_graph* functions.

Yes.

> > acpi_graph* functions in the drivers so we don't have to handle the
> > same thing multiple times with separate APIs. Is it still possible if
> > there is that connector child node?
> 
> Bindings proposed here are OF bindings, I suppose the most important is
> to follow OF specification and guidelines and these bindings tries to
> follow it.
> It looks like it should not be a problem for fwnode framework to handle
> such bindings, but it is just my guess. I have not seen any fwnode*
> specification I am not sure what is the real purpose of this framework,
> but it seems to be just in-kernel abstraction for different firmware
> standards (OF, ACPI), so even if it lacks at the moment some
> functionality it should not be a barrier for OF bindings.

Sure thing.


Thanks,
Rob Herring (Arm) March 5, 2018, 10:14 p.m. UTC | #4
On Tue, Feb 27, 2018 at 08:11:29AM +0100, Andrzej Hajda wrote:
> These bindings allow to describe most known standard USB connectors
> and it should be possible to extend it if necessary.
> USB connectors, beside USB can be used to route other protocols,
> for example UART, Audio, MHL. In such case every device passing data
> through the connector should have appropriate graph bindings.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> v4:
> - improved 'type' description (Rob),
> - improved description of 2nd example (Rob).
> v3:
> - removed MHL port (samsung connector will have separate bindings),
> - added 2nd example for USB-C,
> - improved formatting.
> v2:
> - moved connector type(A,B,C) to compatible string (Rob),
> - renamed size property to type (Rob),
> - changed type description to be less confusing (Laurent),
> - removed vendor specific compatibles (implied by graph port number),
> - added requirement of connector being a child of IC (Rob),
> - removed max-mode (subtly suggested by Rob, it should be detected anyway
>   by USB Controller in runtime, downside is that device is not able to
>   report its real capabilities, maybe better would be to make it optional(?)),
> - assigned port numbers to data buses (Rob).
> 
> Regards
> Andrzej
> ---
>  .../bindings/connector/usb-connector.txt           | 75 ++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt

Reviewed-by: Rob Herring <robh@kernel.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros March 9, 2018, 10:24 a.m. UTC | #5
Hi,

On 27/02/18 09:11, Andrzej Hajda wrote:
> These bindings allow to describe most known standard USB connectors
> and it should be possible to extend it if necessary.
> USB connectors, beside USB can be used to route other protocols,
> for example UART, Audio, MHL. In such case every device passing data
> through the connector should have appropriate graph bindings.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> v4:
> - improved 'type' description (Rob),
> - improved description of 2nd example (Rob).
> v3:
> - removed MHL port (samsung connector will have separate bindings),
> - added 2nd example for USB-C,
> - improved formatting.
> v2:
> - moved connector type(A,B,C) to compatible string (Rob),
> - renamed size property to type (Rob),
> - changed type description to be less confusing (Laurent),
> - removed vendor specific compatibles (implied by graph port number),
> - added requirement of connector being a child of IC (Rob),
> - removed max-mode (subtly suggested by Rob, it should be detected anyway
>   by USB Controller in runtime, downside is that device is not able to
>   report its real capabilities, maybe better would be to make it optional(?)),
> - assigned port numbers to data buses (Rob).
> 
> Regards
> Andrzej
> ---
>  .../bindings/connector/usb-connector.txt           | 75 ++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
> 
> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> new file mode 100644
> index 000000000000..e1463f14af38
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> @@ -0,0 +1,75 @@
> +USB Connector
> +=============
> +
> +USB connector node represents physical USB connector. It should be
> +a child of USB interface controller.
> +
> +Required properties:
> +- compatible: describes type of the connector, must be one of:
> +    "usb-a-connector",
> +    "usb-b-connector",
> +    "usb-c-connector".

compatible should be just "usb-connector"

Type should be a property

type: type of usb connector "A", "B", "AB", "C"
AB is for dual-role connectors.

micro super-speed and high-speed connectors are different. How do you differentiate that?

> +
> +Optional properties:
> +- label: symbolic name for the connector,

Why do you need label? We can't maintain consistency as people will put creative names there.
Device/bus driver could generate a valid label.

> +- type: size of the connector, should be specified in case of USB-A, USB-B
> +  non-fullsize connectors: "mini", "micro".

type is misleading. Type is usually A/B/C. It should be size here instead.

size: size of the connector if not standard size. "mini", "micro"

If not specified it is treated as standard sized connector.
e.g. for Type-C there is no mini/micro. so size doesn't have to be specificed

What about Type-C connector?
> +
> +Required nodes:
> +- any data bus to the connector should be modeled using the OF graph bindings

s/modeled/modelled

> +  specified in bindings/graph.txt, unless the bus is between parent node and
> +  the connector. Since single connector can have multpile data buses every bus

s/multpile/multiple

> +  has assigned OF graph port number as follows:
> +    0: High Speed (HS), present in all connectors,
> +    1: Super Speed (SS), present in SS capable connectors,
> +    2: Sideband use (SBU), present in USB-C.
> +
> +Examples
> +--------
> +
> +1. Micro-USB connector with HS lines routed via controller (MUIC):
> +
> +muic-max77843@66 {
> +	...
> +	usb_con: connector {
> +		compatible = "usb-b-connector";
> +		label = "micro-USB";
> +		type = "micro";
> +	};
> +};
> +
> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
> +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
> +
> +ccic: s2mm005@33 {
> +	...
> +	usb_con: connector {
> +		compatible = "usb-c-connector";
> +		label = "USB-C";

The label is not consistent with the earlier example.

> +
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			port@0 {
> +				reg = <0>;
> +				usb_con_hs: endpoint {
> +					remote-endpoint = <&max77865_usbc_hs>;
> +				};
> +			};
> +			port@1 {
> +				reg = <1>;
> +				usb_con_ss: endpoint {
> +					remote-endpoint = <&usbdrd_phy_ss>;
> +				};
> +			};
> +			port@2 {
> +				reg = <2>;
> +				usb_con_sbu: endpoint {
> +					remote-endpoint = <&dp_aux>;
> +				};
> +			};
> +		};
> +	};
> +};
>
Andrzej Hajda March 12, 2018, 7:02 a.m. UTC | #6
On 09.03.2018 11:24, Roger Quadros wrote:
> Hi,
>
> On 27/02/18 09:11, Andrzej Hajda wrote:
>> These bindings allow to describe most known standard USB connectors
>> and it should be possible to extend it if necessary.
>> USB connectors, beside USB can be used to route other protocols,
>> for example UART, Audio, MHL. In such case every device passing data
>> through the connector should have appropriate graph bindings.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>> v4:
>> - improved 'type' description (Rob),
>> - improved description of 2nd example (Rob).
>> v3:
>> - removed MHL port (samsung connector will have separate bindings),
>> - added 2nd example for USB-C,
>> - improved formatting.
>> v2:
>> - moved connector type(A,B,C) to compatible string (Rob),
>> - renamed size property to type (Rob),
>> - changed type description to be less confusing (Laurent),
>> - removed vendor specific compatibles (implied by graph port number),
>> - added requirement of connector being a child of IC (Rob),
>> - removed max-mode (subtly suggested by Rob, it should be detected anyway
>>   by USB Controller in runtime, downside is that device is not able to
>>   report its real capabilities, maybe better would be to make it optional(?)),
>> - assigned port numbers to data buses (Rob).
>>
>> Regards
>> Andrzej
>> ---
>>  .../bindings/connector/usb-connector.txt           | 75 ++++++++++++++++++++++
>>  1 file changed, 75 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
>>
>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
>> new file mode 100644
>> index 000000000000..e1463f14af38
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>> @@ -0,0 +1,75 @@
>> +USB Connector
>> +=============
>> +
>> +USB connector node represents physical USB connector. It should be
>> +a child of USB interface controller.
>> +
>> +Required properties:
>> +- compatible: describes type of the connector, must be one of:
>> +    "usb-a-connector",
>> +    "usb-b-connector",
>> +    "usb-c-connector".
> compatible should be just "usb-connector"
>
> Type should be a property
>
> type: type of usb connector "A", "B", "AB", "C"
> AB is for dual-role connectors.

I have proposed such property (and size also) in my first RFC [1]. Rod
did not like it :)

[1]: https://marc.info/?l=devicetree&m=150660411515233&w=2


>
> micro super-speed and high-speed connectors are different. How do you differentiate that?

I am aware of it, and property differentiating it was also in my earlier
iterations.
If there will be need to differentiate such connectors, adding property
max-speed or max-mode would do the thing.

>
>> +
>> +Optional properties:
>> +- label: symbolic name for the connector,
> Why do you need label? We can't maintain consistency as people will put creative names there.
> Device/bus driver could generate a valid label.

It follows convention for other connectors: HDMI, DVI, ....

>
>> +- type: size of the connector, should be specified in case of USB-A, USB-B
>> +  non-fullsize connectors: "mini", "micro".
> type is misleading. Type is usually A/B/C. It should be size here instead.

As you can see in discussion for previous iterations it follows
convention already established for other connectors.

>
> size: size of the connector if not standard size. "mini", "micro"
>
> If not specified it is treated as standard sized connector.
> e.g. for Type-C there is no mini/micro. so size doesn't have to be specificed
Few lines above it is described: should be specified in case of USB-A,
USB-B non-fullsize connectors.

>
> What about Type-C connector?
>> +
>> +Required nodes:
>> +- any data bus to the connector should be modeled using the OF graph bindings
> s/modeled/modelled

>> +  specified in bindings/graph.txt, unless the bus is between parent node and
>> +  the connector. Since single connector can have multpile data buses every bus
> s/multpile/multiple

OK, I will fix both typos.

>
>> +  has assigned OF graph port number as follows:
>> +    0: High Speed (HS), present in all connectors,
>> +    1: Super Speed (SS), present in SS capable connectors,
>> +    2: Sideband use (SBU), present in USB-C.
>> +
>> +Examples
>> +--------
>> +
>> +1. Micro-USB connector with HS lines routed via controller (MUIC):
>> +
>> +muic-max77843@66 {
>> +	...
>> +	usb_con: connector {
>> +		compatible = "usb-b-connector";
>> +		label = "micro-USB";
>> +		type = "micro";
>> +	};
>> +};
>> +
>> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
>> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
>> +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
>> +
>> +ccic: s2mm005@33 {
>> +	...
>> +	usb_con: connector {
>> +		compatible = "usb-c-connector";
>> +		label = "USB-C";
> The label is not consistent with the earlier example.

Why?

Regards
Andrzej

>
>> +
>> +		ports {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			port@0 {
>> +				reg = <0>;
>> +				usb_con_hs: endpoint {
>> +					remote-endpoint = <&max77865_usbc_hs>;
>> +				};
>> +			};
>> +			port@1 {
>> +				reg = <1>;
>> +				usb_con_ss: endpoint {
>> +					remote-endpoint = <&usbdrd_phy_ss>;
>> +				};
>> +			};
>> +			port@2 {
>> +				reg = <2>;
>> +				usb_con_sbu: endpoint {
>> +					remote-endpoint = <&dp_aux>;
>> +				};
>> +			};
>> +		};
>> +	};
>> +};
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrzej Hajda March 12, 2018, 7:06 a.m. UTC | #7
On 12.03.2018 08:02, Andrzej Hajda wrote:
> On 09.03.2018 11:24, Roger Quadros wrote:
>> Hi,
>>
>> On 27/02/18 09:11, Andrzej Hajda wrote:
>>> These bindings allow to describe most known standard USB connectors
>>> and it should be possible to extend it if necessary.
>>> USB connectors, beside USB can be used to route other protocols,
>>> for example UART, Audio, MHL. In such case every device passing data
>>> through the connector should have appropriate graph bindings.
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>> v4:
>>> - improved 'type' description (Rob),
>>> - improved description of 2nd example (Rob).
>>> v3:
>>> - removed MHL port (samsung connector will have separate bindings),
>>> - added 2nd example for USB-C,
>>> - improved formatting.
>>> v2:
>>> - moved connector type(A,B,C) to compatible string (Rob),
>>> - renamed size property to type (Rob),
>>> - changed type description to be less confusing (Laurent),
>>> - removed vendor specific compatibles (implied by graph port number),
>>> - added requirement of connector being a child of IC (Rob),
>>> - removed max-mode (subtly suggested by Rob, it should be detected anyway
>>>   by USB Controller in runtime, downside is that device is not able to
>>>   report its real capabilities, maybe better would be to make it optional(?)),
>>> - assigned port numbers to data buses (Rob).
>>>
>>> Regards
>>> Andrzej
>>> ---
>>>  .../bindings/connector/usb-connector.txt           | 75 ++++++++++++++++++++++
>>>  1 file changed, 75 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>> new file mode 100644
>>> index 000000000000..e1463f14af38
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>> @@ -0,0 +1,75 @@
>>> +USB Connector
>>> +=============
>>> +
>>> +USB connector node represents physical USB connector. It should be
>>> +a child of USB interface controller.
>>> +
>>> +Required properties:
>>> +- compatible: describes type of the connector, must be one of:
>>> +    "usb-a-connector",
>>> +    "usb-b-connector",
>>> +    "usb-c-connector".
>> compatible should be just "usb-connector"
>>
>> Type should be a property
>>
>> type: type of usb connector "A", "B", "AB", "C"
>> AB is for dual-role connectors.
> I have proposed such property (and size also) in my first RFC [1]. Rod
> did not like it :)

Ups, ugly typo, I meant Rob, of course.

Regards
Andrzej

>
> [1]: https://marc.info/?l=devicetree&m=150660411515233&w=2
>
>
>> micro super-speed and high-speed connectors are different. How do you differentiate that?
> I am aware of it, and property differentiating it was also in my earlier
> iterations.
> If there will be need to differentiate such connectors, adding property
> max-speed or max-mode would do the thing.
>
>>> +
>>> +Optional properties:
>>> +- label: symbolic name for the connector,
>> Why do you need label? We can't maintain consistency as people will put creative names there.
>> Device/bus driver could generate a valid label.
> It follows convention for other connectors: HDMI, DVI, ....
>
>>> +- type: size of the connector, should be specified in case of USB-A, USB-B
>>> +  non-fullsize connectors: "mini", "micro".
>> type is misleading. Type is usually A/B/C. It should be size here instead.
> As you can see in discussion for previous iterations it follows
> convention already established for other connectors.
>
>> size: size of the connector if not standard size. "mini", "micro"
>>
>> If not specified it is treated as standard sized connector.
>> e.g. for Type-C there is no mini/micro. so size doesn't have to be specificed
> Few lines above it is described: should be specified in case of USB-A,
> USB-B non-fullsize connectors.
>
>> What about Type-C connector?
>>> +
>>> +Required nodes:
>>> +- any data bus to the connector should be modeled using the OF graph bindings
>> s/modeled/modelled
>>> +  specified in bindings/graph.txt, unless the bus is between parent node and
>>> +  the connector. Since single connector can have multpile data buses every bus
>> s/multpile/multiple
> OK, I will fix both typos.
>
>>> +  has assigned OF graph port number as follows:
>>> +    0: High Speed (HS), present in all connectors,
>>> +    1: Super Speed (SS), present in SS capable connectors,
>>> +    2: Sideband use (SBU), present in USB-C.
>>> +
>>> +Examples
>>> +--------
>>> +
>>> +1. Micro-USB connector with HS lines routed via controller (MUIC):
>>> +
>>> +muic-max77843@66 {
>>> +	...
>>> +	usb_con: connector {
>>> +		compatible = "usb-b-connector";
>>> +		label = "micro-USB";
>>> +		type = "micro";
>>> +	};
>>> +};
>>> +
>>> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
>>> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
>>> +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
>>> +
>>> +ccic: s2mm005@33 {
>>> +	...
>>> +	usb_con: connector {
>>> +		compatible = "usb-c-connector";
>>> +		label = "USB-C";
>> The label is not consistent with the earlier example.
> Why?
>
> Regards
> Andrzej
>
>>> +
>>> +		ports {
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +
>>> +			port@0 {
>>> +				reg = <0>;
>>> +				usb_con_hs: endpoint {
>>> +					remote-endpoint = <&max77865_usbc_hs>;
>>> +				};
>>> +			};
>>> +			port@1 {
>>> +				reg = <1>;
>>> +				usb_con_ss: endpoint {
>>> +					remote-endpoint = <&usbdrd_phy_ss>;
>>> +				};
>>> +			};
>>> +			port@2 {
>>> +				reg = <2>;
>>> +				usb_con_sbu: endpoint {
>>> +					remote-endpoint = <&dp_aux>;
>>> +				};
>>> +			};
>>> +		};
>>> +	};
>>> +};
>>>

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros March 12, 2018, 10:41 a.m. UTC | #8
Andrezej,

Why don't you have any of the USB maintainers in to/cc?

Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM)
Felipe Balbi <balbi@kernel.org> (maintainer:USB GADGET/PERIPHERAL SUBSYSTEM)

On 12/03/18 09:02, Andrzej Hajda wrote:
> On 09.03.2018 11:24, Roger Quadros wrote:
>> Hi,
>>
>> On 27/02/18 09:11, Andrzej Hajda wrote:
>>> These bindings allow to describe most known standard USB connectors
>>> and it should be possible to extend it if necessary.
>>> USB connectors, beside USB can be used to route other protocols,
>>> for example UART, Audio, MHL. In such case every device passing data
>>> through the connector should have appropriate graph bindings.
>>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>> v4:
>>> - improved 'type' description (Rob),
>>> - improved description of 2nd example (Rob).
>>> v3:
>>> - removed MHL port (samsung connector will have separate bindings),
>>> - added 2nd example for USB-C,
>>> - improved formatting.
>>> v2:
>>> - moved connector type(A,B,C) to compatible string (Rob),
>>> - renamed size property to type (Rob),
>>> - changed type description to be less confusing (Laurent),
>>> - removed vendor specific compatibles (implied by graph port number),
>>> - added requirement of connector being a child of IC (Rob),
>>> - removed max-mode (subtly suggested by Rob, it should be detected anyway
>>>   by USB Controller in runtime, downside is that device is not able to
>>>   report its real capabilities, maybe better would be to make it optional(?)),
>>> - assigned port numbers to data buses (Rob).
>>>
>>> Regards
>>> Andrzej
>>> ---
>>>  .../bindings/connector/usb-connector.txt           | 75 ++++++++++++++++++++++
>>>  1 file changed, 75 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>> new file mode 100644
>>> index 000000000000..e1463f14af38
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt

Should this lie in  bindings/usb/connector?

>>> @@ -0,0 +1,75 @@
>>> +USB Connector
>>> +=============
>>> +
>>> +USB connector node represents physical USB connector. It should be
>>> +a child of USB interface controller.
>>> +
>>> +Required properties:
>>> +- compatible: describes type of the connector, must be one of:
>>> +    "usb-a-connector",
>>> +    "usb-b-connector",
>>> +    "usb-c-connector".
>> compatible should be just "usb-connector"
>>
>> Type should be a property
>>
>> type: type of usb connector "A", "B", "AB", "C"
>> AB is for dual-role connectors.
> 
> I have proposed such property (and size also) in my first RFC [1]. Rod
> did not like it :)
> 
> [1]: https://marc.info/?l=devicetree&m=150660411515233&w=2
> 

This is what Rob says here https://patchwork.kernel.org/patch/9976043/
"We did "type" for hdmi-connector, but I think I'd really prefer 
compatible be used to distinguish as least where it may matter to s/w. 
In the HDMI case, they all are pretty much the same, just different 
physical size."

So the question is. Does it matter to this particular software implementation
if it is type A,B,C connector?
If yes, how?

Type A will never have any alternate function. It is always dedicated to USB.

Also does the size "full", "micro", "mini" matter to software?

> 
>>
>> micro super-speed and high-speed connectors are different. How do you differentiate that?
> 
> I am aware of it, and property differentiating it was also in my earlier
> iterations.
> If there will be need to differentiate such connectors, adding property
> max-speed or max-mode would do the thing.

USB controller binding (bindings/usb/generic.txt) has the speed.
I don't think there is any value for replicating it for the connector. Maybe it could
be added later if needed.

> 
>>
>>> +
>>> +Optional properties:
>>> +- label: symbolic name for the connector,
>> Why do you need label? We can't maintain consistency as people will put creative names there.
>> Device/bus driver could generate a valid label.
> 
> It follows convention for other connectors: HDMI, DVI, ....
> 
>>
>>> +- type: size of the connector, should be specified in case of USB-A, USB-B
>>> +  non-fullsize connectors: "mini", "micro".
>> type is misleading. Type is usually A/B/C. It should be size here instead.
> 
> As you can see in discussion for previous iterations it follows
> convention already established for other connectors.
> 
>>
>> size: size of the connector if not standard size. "mini", "micro"
>>
>> If not specified it is treated as standard sized connector.
>> e.g. for Type-C there is no mini/micro. so size doesn't have to be specificed
> Few lines above it is described: should be specified in case of USB-A,
> USB-B non-fullsize connectors.
> 
>>
>> What about Type-C connector?
>>> +
>>> +Required nodes:
>>> +- any data bus to the connector should be modeled using the OF graph bindings
>> s/modeled/modelled
> 
>>> +  specified in bindings/graph.txt, unless the bus is between parent node and
>>> +  the connector. Since single connector can have multpile data buses every bus
>> s/multpile/multiple
> 
> OK, I will fix both typos.
> 
>>
>>> +  has assigned OF graph port number as follows:
>>> +    0: High Speed (HS), present in all connectors,
>>> +    1: Super Speed (SS), present in SS capable connectors,
>>> +    2: Sideband use (SBU), present in USB-C.
>>> +
>>> +Examples
>>> +--------
>>> +
>>> +1. Micro-USB connector with HS lines routed via controller (MUIC):
>>> +
>>> +muic-max77843@66 {
>>> +	...
>>> +	usb_con: connector {
>>> +		compatible = "usb-b-connector";
>>> +		label = "micro-USB";
>>> +		type = "micro";
>>> +	};
>>> +};
>>> +
>>> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
>>> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
>>> +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
>>> +
>>> +ccic: s2mm005@33 {
>>> +	...
>>> +	usb_con: connector {
>>> +		compatible = "usb-c-connector";
>>> +		label = "USB-C";
>> The label is not consistent with the earlier example.
> 
> Why?

Because 1st example is "<size>-USB" and second one is "USB-<type>".

How is label going to be used? Is it being presented to end user?
If yes it should indicate what's important to the user. i.e. its function. e.g. "USB-MHL"
or "USB-DisplayPort"

> 
> Regards
> Andrzej
> 
>>
>>> +
>>> +		ports {
>>> +			#address-cells = <1>;
>>> +			#size-cells = <0>;
>>> +
>>> +			port@0 {
>>> +				reg = <0>;
>>> +				usb_con_hs: endpoint {
>>> +					remote-endpoint = <&max77865_usbc_hs>;
>>> +				};
>>> +			};
>>> +			port@1 {
>>> +				reg = <1>;
>>> +				usb_con_ss: endpoint {
>>> +					remote-endpoint = <&usbdrd_phy_ss>;
>>> +				};
>>> +			};
>>> +			port@2 {
>>> +				reg = <2>;
>>> +				usb_con_sbu: endpoint {
>>> +					remote-endpoint = <&dp_aux>;
>>> +				};
>>> +			};
>>> +		};
>>> +	};
>>> +};
>>>
>
Andrzej Hajda March 15, 2018, 11:02 a.m. UTC | #9
On 12.03.2018 11:41, Roger Quadros wrote:
> Andrezej,
>
> Why don't you have any of the USB maintainers in to/cc?
>
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM)
> Felipe Balbi <balbi@kernel.org> (maintainer:USB GADGET/PERIPHERAL SUBSYSTEM)

Serious omission, sorry for that.

>
> On 12/03/18 09:02, Andrzej Hajda wrote:
>> On 09.03.2018 11:24, Roger Quadros wrote:
>>> Hi,
>>>
>>> On 27/02/18 09:11, Andrzej Hajda wrote:
>>>> These bindings allow to describe most known standard USB connectors
>>>> and it should be possible to extend it if necessary.
>>>> USB connectors, beside USB can be used to route other protocols,
>>>> for example UART, Audio, MHL. In such case every device passing data
>>>> through the connector should have appropriate graph bindings.
>>>>
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> ---
>>>> v4:
>>>> - improved 'type' description (Rob),
>>>> - improved description of 2nd example (Rob).
>>>> v3:
>>>> - removed MHL port (samsung connector will have separate bindings),
>>>> - added 2nd example for USB-C,
>>>> - improved formatting.
>>>> v2:
>>>> - moved connector type(A,B,C) to compatible string (Rob),
>>>> - renamed size property to type (Rob),
>>>> - changed type description to be less confusing (Laurent),
>>>> - removed vendor specific compatibles (implied by graph port number),
>>>> - added requirement of connector being a child of IC (Rob),
>>>> - removed max-mode (subtly suggested by Rob, it should be detected anyway
>>>>   by USB Controller in runtime, downside is that device is not able to
>>>>   report its real capabilities, maybe better would be to make it optional(?)),
>>>> - assigned port numbers to data buses (Rob).
>>>>
>>>> Regards
>>>> Andrzej
>>>> ---
>>>>  .../bindings/connector/usb-connector.txt           | 75 ++++++++++++++++++++++
>>>>  1 file changed, 75 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>> new file mode 100644
>>>> index 000000000000..e1463f14af38
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> Should this lie in  bindings/usb/connector?


I guess this is question for DT people. For me both locations are OK.

>
>>>> @@ -0,0 +1,75 @@
>>>> +USB Connector
>>>> +=============
>>>> +
>>>> +USB connector node represents physical USB connector. It should be
>>>> +a child of USB interface controller.
>>>> +
>>>> +Required properties:
>>>> +- compatible: describes type of the connector, must be one of:
>>>> +    "usb-a-connector",
>>>> +    "usb-b-connector",
>>>> +    "usb-c-connector".
>>> compatible should be just "usb-connector"
>>>
>>> Type should be a property
>>>
>>> type: type of usb connector "A", "B", "AB", "C"
>>> AB is for dual-role connectors.
>> I have proposed such property (and size also) in my first RFC [1]. Rod
>> did not like it :)
>>
>> [1]: https://marc.info/?l=devicetree&m=150660411515233&w=2
>>
> This is what Rob says here https://patchwork.kernel.org/patch/9976043/
> "We did "type" for hdmi-connector, but I think I'd really prefer 
> compatible be used to distinguish as least where it may matter to s/w. 
> In the HDMI case, they all are pretty much the same, just different 
> physical size."
>
> So the question is. Does it matter to this particular software implementation
> if it is type A,B,C connector?
> If yes, how?

IMHO both approaches can work from S/W POV. As I understood it moving
usb type to compatible was rather matter of devicetree guidelines I am
not aware of. Again question to Rob.

>
> Type A will never have any alternate function. It is always dedicated to USB.
>
> Also does the size "full", "micro", "mini" matter to software?
>
>>> micro super-speed and high-speed connectors are different. How do you differentiate that?
>> I am aware of it, and property differentiating it was also in my earlier
>> iterations.
>> If there will be need to differentiate such connectors, adding property
>> max-speed or max-mode would do the thing.
> USB controller binding (bindings/usb/generic.txt) has the speed.
> I don't think there is any value for replicating it for the connector. Maybe it could
> be added later if needed.
>
>>>> +
>>>> +Optional properties:
>>>> +- label: symbolic name for the connector,
>>> Why do you need label? We can't maintain consistency as people will put creative names there.
>>> Device/bus driver could generate a valid label.
>> It follows convention for other connectors: HDMI, DVI, ....
>>
>>>> +- type: size of the connector, should be specified in case of USB-A, USB-B
>>>> +  non-fullsize connectors: "mini", "micro".
>>> type is misleading. Type is usually A/B/C. It should be size here instead.
>> As you can see in discussion for previous iterations it follows
>> convention already established for other connectors.
>>
>>> size: size of the connector if not standard size. "mini", "micro"
>>>
>>> If not specified it is treated as standard sized connector.
>>> e.g. for Type-C there is no mini/micro. so size doesn't have to be specificed
>> Few lines above it is described: should be specified in case of USB-A,
>> USB-B non-fullsize connectors.
>>
>>> What about Type-C connector?
>>>> +
>>>> +Required nodes:
>>>> +- any data bus to the connector should be modeled using the OF graph bindings
>>> s/modeled/modelled
>>>> +  specified in bindings/graph.txt, unless the bus is between parent node and
>>>> +  the connector. Since single connector can have multpile data buses every bus
>>> s/multpile/multiple
>> OK, I will fix both typos.
>>
>>>> +  has assigned OF graph port number as follows:
>>>> +    0: High Speed (HS), present in all connectors,
>>>> +    1: Super Speed (SS), present in SS capable connectors,
>>>> +    2: Sideband use (SBU), present in USB-C.
>>>> +
>>>> +Examples
>>>> +--------
>>>> +
>>>> +1. Micro-USB connector with HS lines routed via controller (MUIC):
>>>> +
>>>> +muic-max77843@66 {
>>>> +	...
>>>> +	usb_con: connector {
>>>> +		compatible = "usb-b-connector";
>>>> +		label = "micro-USB";
>>>> +		type = "micro";
>>>> +	};
>>>> +};
>>>> +
>>>> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
>>>> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
>>>> +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
>>>> +
>>>> +ccic: s2mm005@33 {
>>>> +	...
>>>> +	usb_con: connector {
>>>> +		compatible = "usb-c-connector";
>>>> +		label = "USB-C";
>>> The label is not consistent with the earlier example.
>> Why?
> Because 1st example is "<size>-USB" and second one is "USB-<type>".
>
> How is label going to be used? Is it being presented to end user?

I suppose it could be presented to user, and not-interpreted by S/W.

> If yes it should indicate what's important to the user. i.e. its function. e.g. "USB-MHL"
> or "USB-DisplayPort"

Good idea, to emphasize ports capabilities. I guess it could be most
useful in case of multiple USB ports, they could be then named according
to their visible properties/labels, for example: Front-USB, Green-USB,
USB-1.

Regards
Andrzej

>
>> Regards
>> Andrzej
>>
>>>> +
>>>> +		ports {
>>>> +			#address-cells = <1>;
>>>> +			#size-cells = <0>;
>>>> +
>>>> +			port@0 {
>>>> +				reg = <0>;
>>>> +				usb_con_hs: endpoint {
>>>> +					remote-endpoint = <&max77865_usbc_hs>;
>>>> +				};
>>>> +			};
>>>> +			port@1 {
>>>> +				reg = <1>;
>>>> +				usb_con_ss: endpoint {
>>>> +					remote-endpoint = <&usbdrd_phy_ss>;
>>>> +				};
>>>> +			};
>>>> +			port@2 {
>>>> +				reg = <2>;
>>>> +				usb_con_sbu: endpoint {
>>>> +					remote-endpoint = <&dp_aux>;
>>>> +				};
>>>> +			};
>>>> +		};
>>>> +	};
>>>> +};
>>>>

--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Robin Murphy March 15, 2018, 11:46 a.m. UTC | #10
On 12/03/18 10:41, Roger Quadros wrote:
[...]
>>>> @@ -0,0 +1,75 @@
>>>> +USB Connector
>>>> +=============
>>>> +
>>>> +USB connector node represents physical USB connector. It should be
>>>> +a child of USB interface controller.
>>>> +
>>>> +Required properties:
>>>> +- compatible: describes type of the connector, must be one of:
>>>> +    "usb-a-connector",
>>>> +    "usb-b-connector",
>>>> +    "usb-c-connector".
>>> compatible should be just "usb-connector"
>>>
>>> Type should be a property
>>>
>>> type: type of usb connector "A", "B", "AB", "C"
>>> AB is for dual-role connectors.
>>
>> I have proposed such property (and size also) in my first RFC [1]. Rod
>> did not like it :)
>>
>> [1]: https://marc.info/?l=devicetree&m=150660411515233&w=2
>>
> 
> This is what Rob says here https://patchwork.kernel.org/patch/9976043/
> "We did "type" for hdmi-connector, but I think I'd really prefer
> compatible be used to distinguish as least where it may matter to s/w.
> In the HDMI case, they all are pretty much the same, just different
> physical size."
> 
> So the question is. Does it matter to this particular software implementation
> if it is type A,B,C connector?
> If yes, how?
> 
> Type A will never have any alternate function. It is always dedicated to USB.

In USB spec terms, at least. In reality there are things like the cool 
trick Rockchip SoCs do whereby they can expose the debug UART Rx/Tx 
through the OTG port's D+/D- pins, and that is on a type A connector in 
many products. I'm guessing that's probably beyond the scope of this 
binding, though.

> Also does the size "full", "micro", "mini" matter to software?

If it means the user can look in sysfs to easily correlate logical ports 
with physical connectors that's certainly handy (e.g. on something like 
Odroid-XU where the two USB3 ports are brought out to an A and a 
micro-AB connector respectively).

Robin.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros March 15, 2018, 5:08 p.m. UTC | #11
On 15/03/18 13:46, Robin Murphy wrote:
> On 12/03/18 10:41, Roger Quadros wrote:
> [...]
>>>>> @@ -0,0 +1,75 @@
>>>>> +USB Connector
>>>>> +=============
>>>>> +
>>>>> +USB connector node represents physical USB connector. It should be
>>>>> +a child of USB interface controller.
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible: describes type of the connector, must be one of:
>>>>> +    "usb-a-connector",
>>>>> +    "usb-b-connector",
>>>>> +    "usb-c-connector".
>>>> compatible should be just "usb-connector"
>>>>
>>>> Type should be a property
>>>>
>>>> type: type of usb connector "A", "B", "AB", "C"
>>>> AB is for dual-role connectors.
>>>
>>> I have proposed such property (and size also) in my first RFC [1]. Rod
>>> did not like it :)
>>>
>>> [1]: https://marc.info/?l=devicetree&m=150660411515233&w=2
>>>
>>
>> This is what Rob says here https://patchwork.kernel.org/patch/9976043/
>> "We did "type" for hdmi-connector, but I think I'd really prefer
>> compatible be used to distinguish as least where it may matter to s/w.
>> In the HDMI case, they all are pretty much the same, just different
>> physical size."
>>
>> So the question is. Does it matter to this particular software implementation
>> if it is type A,B,C connector?
>> If yes, how?
>>
>> Type A will never have any alternate function. It is always dedicated to USB.
> 
> In USB spec terms, at least. In reality there are things like the cool trick Rockchip SoCs do whereby they can expose the debug UART Rx/Tx through the OTG port's D+/D- pins, and that is on a type A connector in many products. I'm guessing that's probably beyond the scope of this binding, though.
> 
>> Also does the size "full", "micro", "mini" matter to software?
> 
> If it means the user can look in sysfs to easily correlate logical ports with physical connectors that's certainly handy (e.g. on something like Odroid-XU where the two USB3 ports are brought out to an A and a micro-AB connector respectively).

But this logic fails if both connectors are the same type/size.
This is where the label comes in handy. The labels can be unique and end user can identify the port using that.

> 
> Robin.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
new file mode 100644
index 000000000000..e1463f14af38
--- /dev/null
+++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
@@ -0,0 +1,75 @@ 
+USB Connector
+=============
+
+USB connector node represents physical USB connector. It should be
+a child of USB interface controller.
+
+Required properties:
+- compatible: describes type of the connector, must be one of:
+    "usb-a-connector",
+    "usb-b-connector",
+    "usb-c-connector".
+
+Optional properties:
+- label: symbolic name for the connector,
+- type: size of the connector, should be specified in case of USB-A, USB-B
+  non-fullsize connectors: "mini", "micro".
+
+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
+  the connector. Since single connector can have multpile data buses every bus
+  has assigned OF graph port number as follows:
+    0: High Speed (HS), present in all connectors,
+    1: Super Speed (SS), present in SS capable connectors,
+    2: Sideband use (SBU), present in USB-C.
+
+Examples
+--------
+
+1. Micro-USB connector with HS lines routed via controller (MUIC):
+
+muic-max77843@66 {
+	...
+	usb_con: connector {
+		compatible = "usb-b-connector";
+		label = "micro-USB";
+		type = "micro";
+	};
+};
+
+2. USB-C connector attached to CC controller (s2mm005), HS lines routed
+to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
+DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
+
+ccic: s2mm005@33 {
+	...
+	usb_con: connector {
+		compatible = "usb-c-connector";
+		label = "USB-C";
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				usb_con_hs: endpoint {
+					remote-endpoint = <&max77865_usbc_hs>;
+				};
+			};
+			port@1 {
+				reg = <1>;
+				usb_con_ss: endpoint {
+					remote-endpoint = <&usbdrd_phy_ss>;
+				};
+			};
+			port@2 {
+				reg = <2>;
+				usb_con_sbu: endpoint {
+					remote-endpoint = <&dp_aux>;
+				};
+			};
+		};
+	};
+};