[RFC PATCH 0/4] dt-bindings: add bindings for USB physical connector

[RFC PATCH 0/4] dt-bindings: add bindings for USB physical connector

[Andrzej Hajda]

Hi,

This patchset introduces USB connector bindings, together with working example.
I have added comments in relevant patches to describe possible issues.

Regards
Andrzej


Andrzej Hajda (3):
  dt-bindings: add bindings for USB physical connector
  arm64: dts: exynos: add micro-USB connector node to TM2 platforms
  extcon: add possibility to get extcon device by of node

Maciej Purski (1):
  drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL

 .../bindings/connector/usb-connector.txt           | 49 +++++++++++
 .../boot/dts/exynos/exynos5433-tm2-common.dtsi     | 47 ++++++++++-
 drivers/extcon/extcon.c                            | 44 +++++++---
 drivers/gpu/drm/bridge/sil-sii8620.c               | 98 +++++++++++++++++++++-
 include/linux/extcon.h                             |  6 ++
 5 files changed, 228 insertions(+), 16 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/connector/usb-connector.txt

-- 
2.14.1

[RFC PATCH 1/4] dt-bindings: add bindings for USB physical connector

[Andrzej Hajda]

These bindings allows 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>
---
There are few things for discussion (IMO):
1. vendor specific connectors, I have added them here, but maybe better is
   to place them in separate files.
2. physical connector description - I have split it to three properties:
   type(a,b,ab,c), max-mode(ls,fs,hs,ss,ss+), size(mini,micro,powered).
   This tripled is able to describe all USB-standard connectors, but there
   are also impossible combinations, for example(c, *, micro). Maybe better
   would be to just enumerate all possible connectors in include file.
3. Numbering of port/remote nodes, currently only 0 is assigned for Interface
   Controller. Maybe other functions should be also assigned:
   HS, SS, CC, SBU, ... whatever. Maybe functions should be described
   as an additional property of remote node?
...

Regards
Andrzej
---
 .../bindings/connector/usb-connector.txt           | 49 ++++++++++++++++++++++
 1 file changed, 49 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..f3a4e85122d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
@@ -0,0 +1,49 @@
+USB Connector
+=============
+
+Required properties:
+- compatible: "usb-connector"
+  connectors with vendor specific extensions can add one of additional
+  compatibles:
+    "samsung,usb-connector-11pin": 11-pin Samsung micro-USB connector
+- type: the USB connector type: "a", "b", "ab", "c"
+- max-mode: max USB speed mode supported by the connector:
+  "ls", "fs", "hs", "ss", "ss+"
+
+Optional properties:
+- label: a symbolic name for the connector
+- size: size of the connector, should be specified in case of
+  non-standard USB connectors: "mini", "micro", "powered"
+
+Required nodes:
+- any data bus to the connector should be modeled using the
+  OF graph bindings specified in bindings/graph.txt.
+  There should be exactly one port with at least one endpoint to
+  different device nodes. The first endpoint (reg = <0>) should
+  point to USB Interface Controller.
+
+Example
+-------
+
+musb_con: connector {
+	compatible = "samsung,usb-connector-11pin", "usb-connector";
+	label = "usb";
+	type = "b";
+	size = "micro";
+	max-mode = "hs";
+
+	port {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		musb_con_usb_in: endpoint@0 {
+			reg = <0>;
+			remote-endpoint = <&muic_usb_out>;
+		};
+
+		musb_con_mhl_in: endpoint@1 {
+			reg = <1>;
+			remote-endpoint = <&mhl_out>;
+		};
+	};
+};
-- 
2.14.1

Re: [RFC PATCH 1/4] dt-bindings: add bindings for USB physical connector

[Rob Herring]

On Thu, Sep 28, 2017 at 03:07:27PM +0200, Andrzej Hajda wrote:
> These bindings allows 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.

Yay!

> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> There are few things for discussion (IMO):
> 1. vendor specific connectors, I have added them here, but maybe better is
>    to place them in separate files.

I'd worry about the standard connectors first, but probably good to have 
an idea of how vendor connectors need to be extended.

> 2. physical connector description - I have split it to three properties:
>    type(a,b,ab,c), max-mode(ls,fs,hs,ss,ss+), size(mini,micro,powered).
>    This tripled is able to describe all USB-standard connectors, but there
>    are also impossible combinations, for example(c, *, micro). Maybe better
>    would be to just enumerate all possible connectors in include file.

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.

> 3. Numbering of port/remote nodes, currently only 0 is assigned for Interface
>    Controller. Maybe other functions should be also assigned:
>    HS, SS, CC, SBU, ... whatever. Maybe functions should be described
>    as an additional property of remote node?

child of the controller is also an option. There's already prec

> ...
> 
> Regards
> Andrzej
> ---
>  .../bindings/connector/usb-connector.txt           | 49 ++++++++++++++++++++++
>  1 file changed, 49 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..f3a4e85122d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> @@ -0,0 +1,49 @@
> +USB Connector
> +=============
> +
> +Required properties:
> +- compatible: "usb-connector"
> +  connectors with vendor specific extensions can add one of additional
> +  compatibles:
> +    "samsung,usb-connector-11pin": 11-pin Samsung micro-USB connector
> +- type: the USB connector type: "a", "b", "ab", "c"
> +- max-mode: max USB speed mode supported by the connector:
> +  "ls", "fs", "hs", "ss", "ss+"

Do we really see cases where the connector is slower than the 
controller? Plus things like "ls" with an type A connector are not 
valid.

> +
> +Optional properties:
> +- label: a symbolic name for the connector
> +- size: size of the connector, should be specified in case of
> +  non-standard USB connectors: "mini", "micro", "powered"

What does powered mean?

This is really "type" if you compare to HDMI.

> +
> +Required nodes:
> +- any data bus to the connector should be modeled using the
> +  OF graph bindings specified in bindings/graph.txt.
> +  There should be exactly one port with at least one endpoint to
> +  different device nodes. The first endpoint (reg = <0>) should
> +  point to USB Interface Controller.
> +
> +Example
> +-------
> +
> +musb_con: connector {
> +	compatible = "samsung,usb-connector-11pin", "usb-connector";
> +	label = "usb";

> +	type = "b";
> +	size = "micro";
> +	max-mode = "hs";

These all are implied by "samsung,usb-connector-11pin".

> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		musb_con_usb_in: endpoint@0 {
> +			reg = <0>;
> +			remote-endpoint = <&muic_usb_out>;
> +		};
> +
> +		musb_con_mhl_in: endpoint@1 {
> +			reg = <1>;
> +			remote-endpoint = <&mhl_out>;
> +		};

I think this should be 2 ports, not 2 endpoints. Think of ports as 
different data streams and endpoints are either the same data stream 
muxed or fanned out depending on direction. Now for Type-C, 1 port for 
USB and alternate modes is probably correct.

Rob

Re: [RFC PATCH 1/4] dt-bindings: add bindings for USB physical connector

[Andrzej Hajda]

Hi Rob,

Thanks for review.

On 06.10.2017 01:12, Rob Herring wrote:
> On Thu, Sep 28, 2017 at 03:07:27PM +0200, Andrzej Hajda wrote:
>> These bindings allows 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.
> Yay!
>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>> There are few things for discussion (IMO):
>> 1. vendor specific connectors, I have added them here, but maybe better is
>>    to place them in separate files.
> I'd worry about the standard connectors first, but probably good to have 
> an idea of how vendor connectors need to be extended.
>
>> 2. physical connector description - I have split it to three properties:
>>    type(a,b,ab,c), max-mode(ls,fs,hs,ss,ss+), size(mini,micro,powered).
>>    This tripled is able to describe all USB-standard connectors, but there
>>    are also impossible combinations, for example(c, *, micro). Maybe better
>>    would be to just enumerate all possible connectors in include file.
> 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.

I guess that from S/W point of view only matters:
- which IP is connected to which part of the connector, and this can be
handled by port node(s),
- Type: A, B, C
- probably maximal supported speed of the connector - for example SS+
connectors have the same wires but different electrical characteristics
than SS as I understand,

With this in mind maybe we can drop 'type' and introduce following
compatibles:
- usb-a-connector,
- usb-b-connector,
- usb-c-connector.
Encoding other properties in compatible would explode their number, so I
would prefer to avoid it.
I would leave other props:
max-speed: hs, ss, ss+,
(optional)size: micro, mini

I would drop also unpopular/obsolete variants: type-ab, powered, they
can be added later if necessary.

Just for reference, list of connectors defined by USB (>= 2) specifications:
1. USB 2.0 (with later amendments):
- Standard-A plug and receptacle
- Standard-B plug and receptacle
- Mini-B plug and receptacle
- Micro-B plug and receptacle
- Micro-AB receptacle
- Micro-A plug
2. USB 3.0:
- USB 3.0 Standard-A plug and receptacle
- USB 3.0 Standard-B plug and receptacle
- USB 3.0 Powered-B plug and receptacle
- USB 3.0 Micro-B plug and receptacle
- USB 3.0 Micro-A plug
- USB 3.0 Micro-AB receptacle
3. USB 3.1:
- Enhanced SuperSpeed Standard-A plug and receptacle
- Enhanced SuperSpeed Standard-B plug and receptacle
- Enhanced SuperSpeed Micro-B plug and receptacle
- Enhanced SuperSpeed Micro-A plug
- Enhanced SuperSpeed Micro-AB receptacle
4. USB-C (release 1.3):
- USB Full-Featured Type-C receptacle
- USB 2.0 Type-C receptacle
- USB Full-Featured Type-C plug
- USB 2.0 Type-C plug
- USB Type-C Power-Only plug

>> 3. Numbering of port/remote nodes, currently only 0 is assigned for Interface
>>    Controller. Maybe other functions should be also assigned:
>>    HS, SS, CC, SBU, ... whatever. Maybe functions should be described
>>    as an additional property of remote node?
> child of the controller is also an option. There's already prec

Shall we support both solutions or just make one mandatory?

>
>> ...
>>
>> Regards
>> Andrzej
>> ---
>>  .../bindings/connector/usb-connector.txt           | 49 ++++++++++++++++++++++
>>  1 file changed, 49 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..f3a4e85122d5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>> @@ -0,0 +1,49 @@
>> +USB Connector
>> +=============
>> +
>> +Required properties:
>> +- compatible: "usb-connector"
>> +  connectors with vendor specific extensions can add one of additional
>> +  compatibles:
>> +    "samsung,usb-connector-11pin": 11-pin Samsung micro-USB connector
>> +- type: the USB connector type: "a", "b", "ab", "c"
>> +- max-mode: max USB speed mode supported by the connector:
>> +  "ls", "fs", "hs", "ss", "ss+"
> Do we really see cases where the connector is slower than the 
> controller? Plus things like "ls" with an type A connector are not 
> valid.

For example Galaxy Note 4 - it has USB3 controller and micro-USB 2.0
connector.

>
>> +
>> +Optional properties:
>> +- label: a symbolic name for the connector
>> +- size: size of the connector, should be specified in case of
>> +  non-standard USB connectors: "mini", "micro", "powered"
> What does powered mean?

It is standard-B connector with additional power pins, as defined in
USB3.0 spec, we can drop it as not-popular one.


>
> This is really "type" if you compare to HDMI.

As type is already used,  I have to use other word, maybe 'form' would
be better.

>
>> +
>> +Required nodes:
>> +- any data bus to the connector should be modeled using the
>> +  OF graph bindings specified in bindings/graph.txt.
>> +  There should be exactly one port with at least one endpoint to
>> +  different device nodes. The first endpoint (reg = <0>) should
>> +  point to USB Interface Controller.
>> +
>> +Example
>> +-------
>> +
>> +musb_con: connector {
>> +	compatible = "samsung,usb-connector-11pin", "usb-connector";
>> +	label = "usb";
>> +	type = "b";
>> +	size = "micro";
>> +	max-mode = "hs";
> These all are implied by "samsung,usb-connector-11pin".

Yes, but "usb-connector" compatible requires/permits it, or am I wrong ?

>
>> +
>> +	port {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		musb_con_usb_in: endpoint@0 {
>> +			reg = <0>;
>> +			remote-endpoint = <&muic_usb_out>;
>> +		};
>> +
>> +		musb_con_mhl_in: endpoint@1 {
>> +			reg = <1>;
>> +			remote-endpoint = <&mhl_out>;
>> +		};
> I think this should be 2 ports, not 2 endpoints. Think of ports as 
> different data streams and endpoints are either the same data stream 
> muxed or fanned out depending on direction. Now for Type-C, 1 port for 
> USB and alternate modes is probably correct.

I agree for ports.
Regarding type-c, it has separate lines for HS and for SS. HS lines
often go to Interface Controller as they can be used to legacy detection
methods and alternatively muxed to UART.
SS lines go directly to USB3 chip, see for example TPS65982 [1][2]. For
me it is an argument against merging SS and HS lines into one port. What
do you think?
My proposition of port numbering:
0: ID for type-B, CC for type-C
1: HS
2: VBUS - I am not sure if this should be modeled this way, as it is not
a data line,
3: SS
4: SBU

In case of connector being child node of controller some ports can be
omited.

[1]: http://www.ti.com/ods/images/SLVSD02C/pg1_slvsd02.gif
[2]: http://www.ti.com/product/TPS65982/datasheet

Regards
Andrzej

>
> Rob
>
>
>

Re: [RFC PATCH 1/4] dt-bindings: add bindings for USB physical connector

[Rob Herring]

On Fri, Oct 6, 2017 at 6:10 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> Hi Rob,
>
> Thanks for review.
>
> On 06.10.2017 01:12, Rob Herring wrote:
>> On Thu, Sep 28, 2017 at 03:07:27PM +0200, Andrzej Hajda wrote:
>>> These bindings allows 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.
>> Yay!
>>
>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>> ---
>>> There are few things for discussion (IMO):
>>> 1. vendor specific connectors, I have added them here, but maybe better is
>>>    to place them in separate files.
>> I'd worry about the standard connectors first, but probably good to have
>> an idea of how vendor connectors need to be extended.
>>
>>> 2. physical connector description - I have split it to three properties:
>>>    type(a,b,ab,c), max-mode(ls,fs,hs,ss,ss+), size(mini,micro,powered).
>>>    This tripled is able to describe all USB-standard connectors, but there
>>>    are also impossible combinations, for example(c, *, micro). Maybe better
>>>    would be to just enumerate all possible connectors in include file.
>> 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.
>
> I guess that from S/W point of view only matters:
> - which IP is connected to which part of the connector, and this can be
> handled by port node(s),
> - Type: A, B, C
> - probably maximal supported speed of the connector - for example SS+
> connectors have the same wires but different electrical characteristics
> than SS as I understand,
>
> With this in mind maybe we can drop 'type' and introduce following
> compatibles:
> - usb-a-connector,
> - usb-b-connector,
> - usb-c-connector.
> Encoding other properties in compatible would explode their number, so I
> would prefer to avoid it.
> I would leave other props:
> max-speed: hs, ss, ss+,
> (optional)size: micro, mini
>
> I would drop also unpopular/obsolete variants: type-ab, powered, they
> can be added later if necessary.
>
> Just for reference, list of connectors defined by USB (>= 2) specifications:
> 1. USB 2.0 (with later amendments):
> - Standard-A plug and receptacle
> - Standard-B plug and receptacle
> - Mini-B plug and receptacle
> - Micro-B plug and receptacle
> - Micro-AB receptacle
> - Micro-A plug
> 2. USB 3.0:
> - USB 3.0 Standard-A plug and receptacle
> - USB 3.0 Standard-B plug and receptacle
> - USB 3.0 Powered-B plug and receptacle
> - USB 3.0 Micro-B plug and receptacle
> - USB 3.0 Micro-A plug
> - USB 3.0 Micro-AB receptacle
> 3. USB 3.1:
> - Enhanced SuperSpeed Standard-A plug and receptacle
> - Enhanced SuperSpeed Standard-B plug and receptacle
> - Enhanced SuperSpeed Micro-B plug and receptacle
> - Enhanced SuperSpeed Micro-A plug
> - Enhanced SuperSpeed Micro-AB receptacle
> 4. USB-C (release 1.3):
> - USB Full-Featured Type-C receptacle
> - USB 2.0 Type-C receptacle
> - USB Full-Featured Type-C plug
> - USB 2.0 Type-C plug
> - USB Type-C Power-Only plug
>
>>> 3. Numbering of port/remote nodes, currently only 0 is assigned for Interface
>>>    Controller. Maybe other functions should be also assigned:
>>>    HS, SS, CC, SBU, ... whatever. Maybe functions should be described
>>>    as an additional property of remote node?
>> child of the controller is also an option. There's already prec
>
> Shall we support both solutions or just make one mandatory?

Oops, I was just going to say for display, there's already precedent
that ports are used. So that means at least SS should just be
described with ports. Then HS probably should be a port too just to be
symmetrical.

The connector being a child of the USB-PD (or other controller chip)
is probably the only thing that makes sense. I guess the question is
whether there are cases where that doesn't make sense.

>>> ---
>>>  .../bindings/connector/usb-connector.txt           | 49 ++++++++++++++++++++++
>>>  1 file changed, 49 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..f3a4e85122d5
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>> @@ -0,0 +1,49 @@
>>> +USB Connector
>>> +=============
>>> +
>>> +Required properties:
>>> +- compatible: "usb-connector"
>>> +  connectors with vendor specific extensions can add one of additional
>>> +  compatibles:
>>> +    "samsung,usb-connector-11pin": 11-pin Samsung micro-USB connector
>>> +- type: the USB connector type: "a", "b", "ab", "c"
>>> +- max-mode: max USB speed mode supported by the connector:
>>> +  "ls", "fs", "hs", "ss", "ss+"
>> Do we really see cases where the connector is slower than the
>> controller? Plus things like "ls" with an type A connector are not
>> valid.
>
> For example Galaxy Note 4 - it has USB3 controller and micro-USB 2.0
> connector.

How does one distinguish a micro-B SS connector with a HS plug vs. a
SS plug? It must be at run-time. Wouldn't a HS connector behave
operate the same as the former case?

If we do need to distinguish here, then I think it should be by
compatible. Say "usb-ss-b-connector"?

>>> +Optional properties:
>>> +- label: a symbolic name for the connector
>>> +- size: size of the connector, should be specified in case of
>>> +  non-standard USB connectors: "mini", "micro", "powered"
>> What does powered mean?
>
> It is standard-B connector with additional power pins, as defined in
> USB3.0 spec, we can drop it as not-popular one.

Okay. Nothing about it on wikipedia.

>>
>> This is really "type" if you compare to HDMI.
>
> As type is already used,  I have to use other word, maybe 'form' would
> be better.

But you've dropped type in favor of compatible strings, so you can add
it back to replace size and keep the meaning aligned with HDMI.

>>> +
>>> +Required nodes:
>>> +- any data bus to the connector should be modeled using the
>>> +  OF graph bindings specified in bindings/graph.txt.
>>> +  There should be exactly one port with at least one endpoint to
>>> +  different device nodes. The first endpoint (reg = <0>) should
>>> +  point to USB Interface Controller.
>>> +
>>> +Example
>>> +-------
>>> +
>>> +musb_con: connector {
>>> +    compatible = "samsung,usb-connector-11pin", "usb-connector";
>>> +    label = "usb";
>>> +    type = "b";
>>> +    size = "micro";
>>> +    max-mode = "hs";
>> These all are implied by "samsung,usb-connector-11pin".
>
> Yes, but "usb-connector" compatible requires/permits it, or am I wrong ?

Yes, but we're mixing where the compatible implies all the information
and where it's separate fields.

>>> +    port {
>>> +            #address-cells = <1>;
>>> +            #size-cells = <0>;
>>> +
>>> +            musb_con_usb_in: endpoint@0 {
>>> +                    reg = <0>;
>>> +                    remote-endpoint = <&muic_usb_out>;
>>> +            };
>>> +
>>> +            musb_con_mhl_in: endpoint@1 {
>>> +                    reg = <1>;
>>> +                    remote-endpoint = <&mhl_out>;
>>> +            };
>> I think this should be 2 ports, not 2 endpoints. Think of ports as
>> different data streams and endpoints are either the same data stream
>> muxed or fanned out depending on direction. Now for Type-C, 1 port for
>> USB and alternate modes is probably correct.
>
> I agree for ports.
> Regarding type-c, it has separate lines for HS and for SS. HS lines
> often go to Interface Controller as they can be used to legacy detection
> methods and alternatively muxed to UART.
> SS lines go directly to USB3 chip, see for example TPS65982 [1][2]. For
> me it is an argument against merging SS and HS lines into one port. What
> do you think?

If you support USB3 and HDMI/DP alternate modes, there has to be a mux
somewhere. It could be on the SoC or external chip.

> My proposition of port numbering:
> 0: ID for type-B, CC for type-C

I'd expect ID is just a GPIO line, phy pin or goes to some control IC.

> 1: HS
> 2: VBUS - I am not sure if this should be modeled this way, as it is not
> a data line,

Probably not. Pre USB-PD, it's going to be a regulator supply or sink
or goes to some control IC. In the last case, we just need a phandle
reference of some sort. For USB-PD, I'd expect that Vbus, CC, SBU and
perhaps all the SS lines are just routed to some controller. Maybe
just need a similar phandle reference.

> 3: SS
> 4: SBU

Handling these is probably somewhat out of scope of the connector.
HDMI expects to have a "ddc-i2c-bus" and hpd-gpios properties
typically. Where do we put those in this use case?

Rob

Re: [RFC PATCH 1/4] dt-bindings: add bindings for USB physical connector

[Andrzej Hajda]

On 06.10.2017 19:23, Rob Herring wrote:
> On Fri, Oct 6, 2017 at 6:10 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>> Hi Rob,
>>
>> Thanks for review.
>>
>> On 06.10.2017 01:12, Rob Herring wrote:
>>> On Thu, Sep 28, 2017 at 03:07:27PM +0200, Andrzej Hajda wrote:
>>>> These bindings allows 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.
>>> Yay!
>>>
>>>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>>>> ---
>>>> There are few things for discussion (IMO):
>>>> 1. vendor specific connectors, I have added them here, but maybe better is
>>>>    to place them in separate files.
>>> I'd worry about the standard connectors first, but probably good to have
>>> an idea of how vendor connectors need to be extended.
>>>
>>>> 2. physical connector description - I have split it to three properties:
>>>>    type(a,b,ab,c), max-mode(ls,fs,hs,ss,ss+), size(mini,micro,powered).
>>>>    This tripled is able to describe all USB-standard connectors, but there
>>>>    are also impossible combinations, for example(c, *, micro). Maybe better
>>>>    would be to just enumerate all possible connectors in include file.
>>> 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.
>> I guess that from S/W point of view only matters:
>> - which IP is connected to which part of the connector, and this can be
>> handled by port node(s),
>> - Type: A, B, C
>> - probably maximal supported speed of the connector - for example SS+
>> connectors have the same wires but different electrical characteristics
>> than SS as I understand,
>>
>> With this in mind maybe we can drop 'type' and introduce following
>> compatibles:
>> - usb-a-connector,
>> - usb-b-connector,
>> - usb-c-connector.
>> Encoding other properties in compatible would explode their number, so I
>> would prefer to avoid it.
>> I would leave other props:
>> max-speed: hs, ss, ss+,
>> (optional)size: micro, mini
>>
>> I would drop also unpopular/obsolete variants: type-ab, powered, they
>> can be added later if necessary.
>>
>> Just for reference, list of connectors defined by USB (>= 2) specifications:
>> 1. USB 2.0 (with later amendments):
>> - Standard-A plug and receptacle
>> - Standard-B plug and receptacle
>> - Mini-B plug and receptacle
>> - Micro-B plug and receptacle
>> - Micro-AB receptacle
>> - Micro-A plug
>> 2. USB 3.0:
>> - USB 3.0 Standard-A plug and receptacle
>> - USB 3.0 Standard-B plug and receptacle
>> - USB 3.0 Powered-B plug and receptacle
>> - USB 3.0 Micro-B plug and receptacle
>> - USB 3.0 Micro-A plug
>> - USB 3.0 Micro-AB receptacle
>> 3. USB 3.1:
>> - Enhanced SuperSpeed Standard-A plug and receptacle
>> - Enhanced SuperSpeed Standard-B plug and receptacle
>> - Enhanced SuperSpeed Micro-B plug and receptacle
>> - Enhanced SuperSpeed Micro-A plug
>> - Enhanced SuperSpeed Micro-AB receptacle
>> 4. USB-C (release 1.3):
>> - USB Full-Featured Type-C receptacle
>> - USB 2.0 Type-C receptacle
>> - USB Full-Featured Type-C plug
>> - USB 2.0 Type-C plug
>> - USB Type-C Power-Only plug
>>
>>>> 3. Numbering of port/remote nodes, currently only 0 is assigned for Interface
>>>>    Controller. Maybe other functions should be also assigned:
>>>>    HS, SS, CC, SBU, ... whatever. Maybe functions should be described
>>>>    as an additional property of remote node?
>>> child of the controller is also an option. There's already prec
>> Shall we support both solutions or just make one mandatory?
> Oops, I was just going to say for display, there's already precedent
> that ports are used. So that means at least SS should just be
> described with ports. Then HS probably should be a port too just to be
> symmetrical.
>
> The connector being a child of the USB-PD (or other controller chip)
> is probably the only thing that makes sense. I guess the question is
> whether there are cases where that doesn't make sense.
>
>>>> ---
>>>>  .../bindings/connector/usb-connector.txt           | 49 ++++++++++++++++++++++
>>>>  1 file changed, 49 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..f3a4e85122d5
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>>>> @@ -0,0 +1,49 @@
>>>> +USB Connector
>>>> +=============
>>>> +
>>>> +Required properties:
>>>> +- compatible: "usb-connector"
>>>> +  connectors with vendor specific extensions can add one of additional
>>>> +  compatibles:
>>>> +    "samsung,usb-connector-11pin": 11-pin Samsung micro-USB connector
>>>> +- type: the USB connector type: "a", "b", "ab", "c"
>>>> +- max-mode: max USB speed mode supported by the connector:
>>>> +  "ls", "fs", "hs", "ss", "ss+"
>>> Do we really see cases where the connector is slower than the
>>> controller? Plus things like "ls" with an type A connector are not
>>> valid.
>> For example Galaxy Note 4 - it has USB3 controller and micro-USB 2.0
>> connector.
> How does one distinguish a micro-B SS connector with a HS plug vs. a
> SS plug? It must be at run-time.Wouldn't a HS connector behave
> operate the same as the former case?
>
> If we do need to distinguish here, then I think it should be by
> compatible. Say "usb-ss-b-connector"?

I guess that from S/W POV we may need it only in corner cases,
but it could be also good to have this distinction even for informative
purposes.

But if we want to encode it in compatible we can end up with:
usb-a-connector
usb-ss-a-connector
usb-ssplus-a-connector
usb-b-connector
usb-ss-b-connector
usb-ssplus-b-connector
usb-c-connector
usb-ss-c-connector

I see no big problem with it, but I do not see advantage over separate
property 'max-speed', or maybe better 'max-mode'.
I suppose I miss some important DT principles, what is the rationale
behind encoding such things in compatibles.

>
>>>> +Optional properties:
>>>> +- label: a symbolic name for the connector
>>>> +- size: size of the connector, should be specified in case of
>>>> +  non-standard USB connectors: "mini", "micro", "powered"
>>> What does powered mean?
>> It is standard-B connector with additional power pins, as defined in
>> USB3.0 spec, we can drop it as not-popular one.
> Okay. Nothing about it on wikipedia.
>
>>> This is really "type" if you compare to HDMI.
>> As type is already used,  I have to use other word, maybe 'form' would
>> be better.
> But you've dropped type in favor of compatible strings, so you can add
> it back to replace size and keep the meaning aligned with HDMI.


OK.

>
>>>> +
>>>> +Required nodes:
>>>> +- any data bus to the connector should be modeled using the
>>>> +  OF graph bindings specified in bindings/graph.txt.
>>>> +  There should be exactly one port with at least one endpoint to
>>>> +  different device nodes. The first endpoint (reg = <0>) should
>>>> +  point to USB Interface Controller.
>>>> +
>>>> +Example
>>>> +-------
>>>> +
>>>> +musb_con: connector {
>>>> +    compatible = "samsung,usb-connector-11pin", "usb-connector";
>>>> +    label = "usb";
>>>> +    type = "b";
>>>> +    size = "micro";
>>>> +    max-mode = "hs";
>>> These all are implied by "samsung,usb-connector-11pin".
>> Yes, but "usb-connector" compatible requires/permits it, or am I wrong ?
> Yes, but we're mixing where the compatible implies all the information
> and where it's separate fields.

So how 11-pin connector should be described?

>
>>>> +    port {
>>>> +            #address-cells = <1>;
>>>> +            #size-cells = <0>;
>>>> +
>>>> +            musb_con_usb_in: endpoint@0 {
>>>> +                    reg = <0>;
>>>> +                    remote-endpoint = <&muic_usb_out>;
>>>> +            };
>>>> +
>>>> +            musb_con_mhl_in: endpoint@1 {
>>>> +                    reg = <1>;
>>>> +                    remote-endpoint = <&mhl_out>;
>>>> +            };
>>> I think this should be 2 ports, not 2 endpoints. Think of ports as
>>> different data streams and endpoints are either the same data stream
>>> muxed or fanned out depending on direction. Now for Type-C, 1 port for
>>> USB and alternate modes is probably correct.
>> I agree for ports.
>> Regarding type-c, it has separate lines for HS and for SS. HS lines
>> often go to Interface Controller as they can be used to legacy detection
>> methods and alternatively muxed to UART.
>> SS lines go directly to USB3 chip, see for example TPS65982 [1][2]. For
>> me it is an argument against merging SS and HS lines into one port. What
>> do you think?
> If you support USB3 and HDMI/DP alternate modes, there has to be a mux
> somewhere. It could be on the SoC or external chip.

Yes. In the device I am working on it is in USB3 phy, but this mux is
only for SS lines.

>
>> My proposition of port numbering:
>> 0: ID for type-B, CC for type-C
> I'd expect ID is just a GPIO line, phy pin or goes to some control IC.

Yes, ID and CC should not be described by graph at all if the connector
is the child of InterfaceController.

>
>> 1: HS
>> 2: VBUS - I am not sure if this should be modeled this way, as it is not
>> a data line,
> Probably not. Pre USB-PD, it's going to be a regulator supply or sink
> or goes to some control IC. In the last case, we just need a phandle
> reference of some sort. For USB-PD, I'd expect that Vbus, CC, SBU and
> perhaps all the SS lines are just routed to some controller. Maybe
> just need a similar phandle reference.

Yes, phandle is something more appropriate here, I am not sure only how
it will work with current kernel frameworks, but this is different issue.

>
>> 3: SS
>> 4: SBU
> Handling these is probably somewhat out of scope of the connector.

Why? This is integral part of the connector.

> HDMI expects to have a "ddc-i2c-bus" and hpd-gpios properties
> typically. Where do we put those in this use case?

In case of USB-C DP Alt mode:
- HPD is encapsulated in USB-PD messages,
- DDC is encapsulated in DP-AUX channel which goes via SBU lines,
In case of USB-C HDMI Alt-mode:
- DDC is encapsulated in USB-PD messages,
- HPD is passed together with HEAC and utility via SBU lines.
In USB-A, USB-B, USB-C without alt modes these signals does not make sense.

Do we need ddc-i2c-bus and hpd-gpios properties in USB connector?

Regards
Andrzej

Re: [RFC PATCH 1/4] dt-bindings: add bindings for USB physical connector

[Laurent Pinchart]

Hi Andrzej,

Thank you for the patch.

On Thursday, 28 September 2017 16:07:27 EEST Andrzej Hajda wrote:
> These bindings allows 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>
> ---
> There are few things for discussion (IMO):
> 1. vendor specific connectors, I have added them here, but maybe better is
>    to place them in separate files.

It's useful to have one vendor-specific compatible string to be used in the 
example. We could split vendor-specific connectors to separate files later if 
needed, but for now I'm fine keeping them here.

> 2. physical connector description - I have split it to three properties:
>    type(a,b,ab,c), max-mode(ls,fs,hs,ss,ss+), size(mini,micro,powered).
>    This tripled is able to describe all USB-standard connectors, but there
>    are also impossible combinations, for example(c, *, micro). Maybe better
>    would be to just enumerate all possible connectors in include file.

I don't have a strong opinion on this. The three properties are nicely 
descriptive. You might want to list the valid combinations in the bindings 
though.

> 3. Numbering of port/remote nodes, currently only 0 is assigned for
> Interface Controller. Maybe other functions should be also assigned:
>    HS, SS, CC, SBU, ... whatever. Maybe functions should be described
>    as an additional property of remote node?

Given that one of the main reasons this binding is needed is to describe MHL 
connection to a USB connector, I think we'll need to define additional 
functions, yes. I'm not sure yet how that should look like though.

> ---
>  .../bindings/connector/usb-connector.txt           | 49 +++++++++++++++++++
>  1 file changed, 49 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..f3a4e85122d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> @@ -0,0 +1,49 @@
> +USB Connector
> +=============
> +
> +Required properties:
> +- compatible: "usb-connector"
> +  connectors with vendor specific extensions can add one of additional
> +  compatibles:
> +    "samsung,usb-connector-11pin": 11-pin Samsung micro-USB connector
> +- type: the USB connector type: "a", "b", "ab", "c"
> +- max-mode: max USB speed mode supported by the connector:
> +  "ls", "fs", "hs", "ss", "ss+"
> +
> +Optional properties:
> +- label: a symbolic name for the connector
> +- size: size of the connector, should be specified in case of
> +  non-standard USB connectors: "mini", "micro", "powered"

"non-standard" sounds like "vendor-specific", while I assume you're talking 
about the size. The USB specification uses the term "standard" for this 
purpose, so it's hard to use another one that would convey the right meaning 
precisely. Maybe "non-standard ('large') USB connector sizes" ?

> +Required nodes:
> +- any data bus to the connector should be modeled using the
> +  OF graph bindings specified in bindings/graph.txt.
> +  There should be exactly one port with at least one endpoint to
> +  different device nodes. The first endpoint (reg = <0>) should
> +  point to USB Interface Controller.
> +
> +Example
> +-------
> +
> +musb_con: connector {
> +	compatible = "samsung,usb-connector-11pin", "usb-connector";
> +	label = "usb";
> +	type = "b";
> +	size = "micro";
> +	max-mode = "hs";
> +
> +	port {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		musb_con_usb_in: endpoint@0 {
> +			reg = <0>;
> +			remote-endpoint = <&muic_usb_out>;
> +		};
> +
> +		musb_con_mhl_in: endpoint@1 {
> +			reg = <1>;
> +			remote-endpoint = <&mhl_out>;
> +		};
> +	};
> +};

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/4] dt-bindings: add bindings for USB physical connector

[Laurent Pinchart]

Hi Andrzej,

On Wednesday, 18 October 2017 18:11:25 EEST Laurent Pinchart wrote:
> On Thursday, 28 September 2017 16:07:27 EEST Andrzej Hajda wrote:
> > These bindings allows 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>
> > ---
> > There are few things for discussion (IMO):
> > 1. vendor specific connectors, I have added them here, but maybe better is
> > 
> >    to place them in separate files.
> 
> It's useful to have one vendor-specific compatible string to be used in the
> example. We could split vendor-specific connectors to separate files later
> if needed, but for now I'm fine keeping them here.
> 
> > 2. physical connector description - I have split it to three properties:
> >    type(a,b,ab,c), max-mode(ls,fs,hs,ss,ss+), size(mini,micro,powered).
> >    This tripled is able to describe all USB-standard connectors, but there
> >    are also impossible combinations, for example(c, *, micro). Maybe
> >    better
> >    would be to just enumerate all possible connectors in include file.
> 
> I don't have a strong opinion on this. The three properties are nicely
> descriptive. You might want to list the valid combinations in the bindings
> though.
> 
> > 3. Numbering of port/remote nodes, currently only 0 is assigned for
> > 
> > Interface Controller. Maybe other functions should be also assigned:
> >    HS, SS, CC, SBU, ... whatever. Maybe functions should be described
> >    as an additional property of remote node?
> 
> Given that one of the main reasons this binding is needed is to describe MHL
> connection to a USB connector, I think we'll need to define additional
> functions, yes. I'm not sure yet how that should look like though.
> > ---
> > 
> >  .../bindings/connector/usb-connector.txt           | 49 +++++++++++++++++
> >  1 file changed, 49 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..f3a4e85122d5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> > @@ -0,0 +1,49 @@
> > +USB Connector
> > +=============
> > +
> > +Required properties:
> > +- compatible: "usb-connector"
> > +  connectors with vendor specific extensions can add one of additional
> > +  compatibles:
> > +    "samsung,usb-connector-11pin": 11-pin Samsung micro-USB connector
> > +- type: the USB connector type: "a", "b", "ab", "c"
> > +- max-mode: max USB speed mode supported by the connector:
> > +  "ls", "fs", "hs", "ss", "ss+"
> > +
> > +Optional properties:
> > +- label: a symbolic name for the connector
> > +- size: size of the connector, should be specified in case of
> > +  non-standard USB connectors: "mini", "micro", "powered"
> 
> "non-standard" sounds like "vendor-specific", while I assume you're talking
> about the size. The USB specification uses the term "standard" for this
> purpose, so it's hard to use another one that would convey the right meaning
> precisely. Maybe "non-standard ('large') USB connector sizes" ?
> 
> > +Required nodes:
> > +- any data bus to the connector should be modeled using the
> > +  OF graph bindings specified in bindings/graph.txt.
> > +  There should be exactly one port with at least one endpoint to
> > +  different device nodes. The first endpoint (reg = <0>) should
> > +  point to USB Interface Controller.
> > +
> > +Example
> > +-------
> > +
> > +musb_con: connector {
> > +	compatible = "samsung,usb-connector-11pin", "usb-connector";
> > +	label = "usb";
> > +	type = "b";
> > +	size = "micro";
> > +	max-mode = "hs";
> > +
> > +	port {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +
> > +		musb_con_usb_in: endpoint@0 {
> > +			reg = <0>;
> > +			remote-endpoint = <&muic_usb_out>;
> > +		};
> > +
> > +		musb_con_mhl_in: endpoint@1 {
> > +			reg = <1>;
> > +			remote-endpoint = <&mhl_out>;
> > +		};
> > +	};
> > +};

One more comment, do I assume correctly that the Samsung 11-pin connector 
carries USB and MHL on different pins ?

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/4] dt-bindings: add bindings for USB physical connector

[Andrzej Hajda]

Hi Laurent,

Thank you for the review.

On 18.10.2017 17:11, Laurent Pinchart wrote:
> Hi Andrzej,
>
> Thank you for the patch.
>
> On Thursday, 28 September 2017 16:07:27 EEST Andrzej Hajda wrote:
>> These bindings allows 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>
>> ---
>> There are few things for discussion (IMO):
>> 1. vendor specific connectors, I have added them here, but maybe better is
>>    to place them in separate files.
> It's useful to have one vendor-specific compatible string to be used in the 
> example. We could split vendor-specific connectors to separate files later if 
> needed, but for now I'm fine keeping them here.
>
>> 2. physical connector description - I have split it to three properties:
>>    type(a,b,ab,c), max-mode(ls,fs,hs,ss,ss+), size(mini,micro,powered).
>>    This tripled is able to describe all USB-standard connectors, but there
>>    are also impossible combinations, for example(c, *, micro). Maybe better
>>    would be to just enumerate all possible connectors in include file.
> I don't have a strong opinion on this. The three properties are nicely 
> descriptive. You might want to list the valid combinations in the bindings 
> though.

According to Rob's suggestion in next iteration I will encode USB port
type into compatible ie:
- usb-a-connector,
- usb-b-connector,
- usb-c-connector.

Rob suggested also to encode there speed as well, but I am afraid it
will inflate number of compatibles:
(3 types: a, b, c) x (3 speed modes: hs, ss, ssplus) = 9 combinations

>> 3. Numbering of port/remote nodes, currently only 0 is assigned for
>> Interface Controller. Maybe other functions should be also assigned:
>>    HS, SS, CC, SBU, ... whatever. Maybe functions should be described
>>    as an additional property of remote node?
> Given that one of the main reasons this binding is needed is to describe MHL 
> connection to a USB connector, I think we'll need to define additional 
> functions, yes. I'm not sure yet how that should look like though.

Current idea is to encode it in port number.

>
>> ---
>>  .../bindings/connector/usb-connector.txt           | 49 +++++++++++++++++++
>>  1 file changed, 49 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..f3a4e85122d5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
>> @@ -0,0 +1,49 @@
>> +USB Connector
>> +=============
>> +
>> +Required properties:
>> +- compatible: "usb-connector"
>> +  connectors with vendor specific extensions can add one of additional
>> +  compatibles:
>> +    "samsung,usb-connector-11pin": 11-pin Samsung micro-USB connector
>> +- type: the USB connector type: "a", "b", "ab", "c"
>> +- max-mode: max USB speed mode supported by the connector:
>> +  "ls", "fs", "hs", "ss", "ss+"
>> +
>> +Optional properties:
>> +- label: a symbolic name for the connector
>> +- size: size of the connector, should be specified in case of
>> +  non-standard USB connectors: "mini", "micro", "powered"
> "non-standard" sounds like "vendor-specific", while I assume you're talking 
> about the size. The USB specification uses the term "standard" for this 
> purpose, so it's hard to use another one that would convey the right meaning 
> precisely. Maybe "non-standard ('large') USB connector sizes" ?

OK.

And answer for your question from other e-mail:
> One more comment, do I assume correctly that the Samsung 11-pin connector 
> carries USB and MHL on different pins ?

Yes, there are three additional pins: MHL_DP, MHL_DN and MHL_ID [1].

[1]:
https://ae01.alicdn.com/kf/HTB1nn.6KVXXXXaNXFXXq6xXFXXXc/221542210/HTB1nn.6KVXXXXaNXFXXq6xXFXXXc.jpg?size=69211&height=700&width=700&hash=dcababf11610a489d451d7cb0b8ab60e

Thanks
Andrzej

>
>> +Required nodes:
>> +- any data bus to the connector should be modeled using the
>> +  OF graph bindings specified in bindings/graph.txt.
>> +  There should be exactly one port with at least one endpoint to
>> +  different device nodes. The first endpoint (reg = <0>) should
>> +  point to USB Interface Controller.
>> +
>> +Example
>> +-------
>> +
>> +musb_con: connector {
>> +	compatible = "samsung,usb-connector-11pin", "usb-connector";
>> +	label = "usb";
>> +	type = "b";
>> +	size = "micro";
>> +	max-mode = "hs";
>> +
>> +	port {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		musb_con_usb_in: endpoint@0 {
>> +			reg = <0>;
>> +			remote-endpoint = <&muic_usb_out>;
>> +		};
>> +
>> +		musb_con_mhl_in: endpoint@1 {
>> +			reg = <1>;
>> +			remote-endpoint = <&mhl_out>;
>> +		};
>> +	};
>> +};

[RFC PATCH 2/4] arm64: dts: exynos: add micro-USB connector node to TM2 platforms

[Andrzej Hajda]

Since USB connector bindings are available we can describe it on TM2(e).

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 .../boot/dts/exynos/exynos5433-tm2-common.dtsi     | 47 ++++++++++++++++++++--
 1 file changed, 44 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
index e30bae4cf878..39b1ca0ef4cd 100644
--- a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
+++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
@@ -135,6 +135,28 @@
 			"RCV", "HPOUT3R";
 		status = "okay";
 	};
+
+	musb_con: musb_connector {
+		compatible = "samsung,usb-connector-11pin", "usb-connector";
+		label = "micro-usb";
+		type = "b";
+		size = "micro";
+		max-mode = "hs";
+
+		port {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			musb_con_to_muic: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&muic_to_musb_con>;
+			};
+			musb_con_to_mhl: endpoint@1 {
+				reg = <1>;
+				remote-endpoint = <&mhl_to_musb_con>;
+			};
+		};
+	};
 };
 
 &adc {
@@ -768,9 +790,22 @@
 		clocks = <&pmu_system_controller 0>;
 		clock-names = "xtal";
 
-		port {
-			mhl_to_hdmi: endpoint {
-				remote-endpoint = <&hdmi_to_mhl>;
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				mhl_to_hdmi: endpoint {
+					remote-endpoint = <&hdmi_to_mhl>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				mhl_to_musb_con: endpoint {
+					remote-endpoint = <&musb_con_to_mhl>;
+				};
 			};
 		};
 	};
@@ -787,6 +822,12 @@
 
 		muic: max77843-muic {
 			compatible = "maxim,max77843-muic";
+
+			port {
+				muic_to_musb_con: endpoint@0 {
+					remote-endpoint = <&musb_con_to_muic>;
+				};
+			};
 		};
 
 		regulators {
-- 
2.14.1

[RFC PATCH 3/4] extcon: add possibility to get extcon device by of node

[Andrzej Hajda]

Since extcon property is not allowed in DT, extcon subsystem requires
another way to get extcon device. Lets try the simplest approach - get
edev by of_node.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/extcon/extcon.c | 44 ++++++++++++++++++++++++++++++++++----------
 include/linux/extcon.h  |  6 ++++++
 2 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index cb38c2747684..fdb8c1d767c1 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1336,6 +1336,28 @@ void extcon_dev_unregister(struct extcon_dev *edev)
 EXPORT_SYMBOL_GPL(extcon_dev_unregister);
 
 #ifdef CONFIG_OF
+
+/*
+ * extcon_get_edev_by_of_node - Get the extcon device from devicetree.
+ * @node	: OF node identyfying edev
+ *
+ * Return the pointer of extcon device if success or ERR_PTR(err) if fail.
+ */
+struct extcon_dev *extcon_get_edev_by_of_node(struct device_node *node)
+{
+	struct extcon_dev *edev;
+
+	mutex_lock(&extcon_dev_list_lock);
+	list_for_each_entry(edev, &extcon_dev_list, entry)
+		if (edev->dev.parent && edev->dev.parent->of_node == node)
+			goto end;
+	edev = ERR_PTR(-EPROBE_DEFER);
+end:
+	mutex_unlock(&extcon_dev_list_lock);
+
+	return edev;
+}
+
 /*
  * extcon_get_edev_by_phandle - Get the extcon device from devicetree.
  * @dev		: the instance to the given device
@@ -1363,25 +1385,27 @@ struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index)
 		return ERR_PTR(-ENODEV);
 	}
 
-	mutex_lock(&extcon_dev_list_lock);
-	list_for_each_entry(edev, &extcon_dev_list, entry) {
-		if (edev->dev.parent && edev->dev.parent->of_node == node) {
-			mutex_unlock(&extcon_dev_list_lock);
-			of_node_put(node);
-			return edev;
-		}
-	}
-	mutex_unlock(&extcon_dev_list_lock);
+	edev = extcon_get_edev_by_of_node(node);
 	of_node_put(node);
 
-	return ERR_PTR(-EPROBE_DEFER);
+	return edev;
 }
+
 #else
+
+struct extcon_dev *extcon_get_edev_by_of_node(struct device_node *node)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index)
 {
 	return ERR_PTR(-ENOSYS);
 }
+
 #endif /* CONFIG_OF */
+
+EXPORT_SYMBOL_GPL(extcon_get_edev_by_of_node);
 EXPORT_SYMBOL_GPL(extcon_get_edev_by_phandle);
 
 /**
diff --git a/include/linux/extcon.h b/include/linux/extcon.h
index 744d60ca80c3..2f88e7491672 100644
--- a/include/linux/extcon.h
+++ b/include/linux/extcon.h
@@ -261,6 +261,7 @@ extern void devm_extcon_unregister_notifier_all(struct device *dev,
  * Following APIs get the extcon_dev from devicetree or by through extcon name.
  */
 extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
+extern struct extcon_dev *extcon_get_edev_by_of_node(struct device_node *node);
 extern struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev,
 						     int index);
 
@@ -382,6 +383,11 @@ static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
 	return ERR_PTR(-ENODEV);
 }
 
+static inline struct extcon_dev *extcon_get_edev_by_of_node(struct device_node *node)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 static inline struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev,
 				int index)
 {
-- 
2.14.1

Re: [RFC PATCH 3/4] extcon: add possibility to get extcon device by of node

[Chanwoo Choi]

Hi,

On 2017년 09월 28일 22:07, Andrzej Hajda wrote:
> Since extcon property is not allowed in DT, extcon subsystem requires
> another way to get extcon device. Lets try the simplest approach - get
> edev by of_node.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/extcon/extcon.c | 44 ++++++++++++++++++++++++++++++++++----------
>  include/linux/extcon.h  |  6 ++++++
>  2 files changed, 40 insertions(+), 10 deletions(-)

Looks good to me. Just I added the minor comment.
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

> 
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index cb38c2747684..fdb8c1d767c1 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -1336,6 +1336,28 @@ void extcon_dev_unregister(struct extcon_dev *edev)
>  EXPORT_SYMBOL_GPL(extcon_dev_unregister);
>  
>  #ifdef CONFIG_OF
> +
> +/*
> + * extcon_get_edev_by_of_node - Get the extcon device from devicetree.
> + * @node	: OF node identyfying edev
> + *
> + * Return the pointer of extcon device if success or ERR_PTR(err) if fail.
> + */
> +struct extcon_dev *extcon_get_edev_by_of_node(struct device_node *node)
> +{
> +	struct extcon_dev *edev;
> +
> +	mutex_lock(&extcon_dev_list_lock);
> +	list_for_each_entry(edev, &extcon_dev_list, entry)
> +		if (edev->dev.parent && edev->dev.parent->of_node == node)
> +			goto end;
> +	edev = ERR_PTR(-EPROBE_DEFER);
> +end:

The extcon.c already use the 'out' statement for goto.
I'd like you to use 'out' instead of 'end'.

> +	mutex_unlock(&extcon_dev_list_lock);
> +
> +	return edev;
> +}
> +
>  /*
>   * extcon_get_edev_by_phandle - Get the extcon device from devicetree.
>   * @dev		: the instance to the given device
> @@ -1363,25 +1385,27 @@ struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index)
>  		return ERR_PTR(-ENODEV);
>  	}
>  
> -	mutex_lock(&extcon_dev_list_lock);
> -	list_for_each_entry(edev, &extcon_dev_list, entry) {
> -		if (edev->dev.parent && edev->dev.parent->of_node == node) {
> -			mutex_unlock(&extcon_dev_list_lock);
> -			of_node_put(node);
> -			return edev;
> -		}
> -	}
> -	mutex_unlock(&extcon_dev_list_lock);
> +	edev = extcon_get_edev_by_of_node(node);
>  	of_node_put(node);
>  
> -	return ERR_PTR(-EPROBE_DEFER);
> +	return edev;
>  }
> +
>  #else
> +
> +struct extcon_dev *extcon_get_edev_by_of_node(struct device_node *node)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
> +
>  struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev, int index)
>  {
>  	return ERR_PTR(-ENOSYS);
>  }
> +
>  #endif /* CONFIG_OF */
> +
> +EXPORT_SYMBOL_GPL(extcon_get_edev_by_of_node);
>  EXPORT_SYMBOL_GPL(extcon_get_edev_by_phandle);
>  
>  /**
> diff --git a/include/linux/extcon.h b/include/linux/extcon.h
> index 744d60ca80c3..2f88e7491672 100644
> --- a/include/linux/extcon.h
> +++ b/include/linux/extcon.h
> @@ -261,6 +261,7 @@ extern void devm_extcon_unregister_notifier_all(struct device *dev,
>   * Following APIs get the extcon_dev from devicetree or by through extcon name.
>   */
>  extern struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name);
> +extern struct extcon_dev *extcon_get_edev_by_of_node(struct device_node *node);
>  extern struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev,
>  						     int index);
>  
> @@ -382,6 +383,11 @@ static inline struct extcon_dev *extcon_get_extcon_dev(const char *extcon_name)
>  	return ERR_PTR(-ENODEV);
>  }
>  
> +static inline struct extcon_dev *extcon_get_edev_by_of_node(struct device_node *node)
> +{
> +	return ERR_PTR(-ENODEV);
> +}
> +
>  static inline struct extcon_dev *extcon_get_edev_by_phandle(struct device *dev,
>  				int index)
>  {
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

[RFC PATCH 4/4] drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL

[Andrzej Hajda]

From: Maciej Purski <m.purski@samsung.com>

Currently MHL chip must be turned on permanently to detect MHL cable. It
duplicates micro-USB controller's (MUIC) functionality and consumes
unnecessary power. Lets use extcon attached to MUIC to enable MHL chip
only if it detects MHL cable.

Signed-off-by: Maciej Purski <m.purski@samsung.com>
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
This is rework of the patch by Maciej with following changes:
- use micro-USB port bindings to get extcon, instead of extcon property,
- fixed remove sequence,
- fixed extcon get state logic.

Code finding extcon node is hacky IMO, I guess ultimately it should be done
via some framework (maybe even extcon), or at least via helper, I hope it
can stay as is until the proper solution will be merged.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/bridge/sil-sii8620.c | 98 ++++++++++++++++++++++++++++++++++--
 1 file changed, 95 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index 5131bfb94f06..6f40cbc2445e 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -17,6 +17,7 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/extcon.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -25,6 +26,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of_graph.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
@@ -78,6 +80,10 @@ struct sii8620 {
 	struct edid *edid;
 	unsigned int gen2_write_burst:1;
 	enum sii8620_mt_state mt_state;
+	struct extcon_dev *extcon;
+	struct notifier_block extcon_nb;
+	struct work_struct extcon_wq;
+	int cable_state;
 	struct list_head mt_queue;
 	struct {
 		int r_size;
@@ -2102,6 +2108,78 @@ static void sii8620_cable_in(struct sii8620 *ctx)
 	enable_irq(to_i2c_client(ctx->dev)->irq);
 }
 
+static void sii8620_cable_out(struct sii8620 *ctx)
+{
+	disable_irq(to_i2c_client(ctx->dev)->irq);
+	sii8620_hw_off(ctx);
+}
+
+static void sii8620_extcon_work(struct work_struct *work)
+{
+	struct sii8620 *ctx =
+		container_of(work, struct sii8620, extcon_wq);
+	int state = extcon_get_state(ctx->extcon, EXTCON_DISP_MHL);
+
+	if (state == ctx->cable_state)
+		return;
+
+	ctx->cable_state = state;
+
+	if (state > 0)
+		sii8620_cable_in(ctx);
+	else
+		sii8620_cable_out(ctx);
+}
+
+static int sii8620_extcon_notifier(struct notifier_block *self,
+			unsigned long event, void *ptr)
+{
+	struct sii8620 *ctx =
+		container_of(self, struct sii8620, extcon_nb);
+
+	schedule_work(&ctx->extcon_wq);
+
+	return NOTIFY_DONE;
+}
+
+static int sii8620_extcon_init(struct sii8620 *ctx)
+{
+	struct extcon_dev *edev;
+	struct device_node *musb, *muic;
+	int ret;
+
+	/* get micro-USB connector node */
+	musb = of_graph_get_remote_node(ctx->dev->of_node, 1, -1);
+	/* next get micro-USB Interface Controller node */
+	muic = of_graph_get_remote_node(musb, -1, 0);
+	of_node_put(musb);
+
+	if (!muic) {
+		dev_info(ctx->dev, "no extcon found, switching to 'always on' mode\n");
+		return 0;
+	}
+
+	edev = extcon_get_edev_by_of_node(muic);
+	of_node_put(muic);
+	if (IS_ERR(edev)) {
+		if (PTR_ERR(edev) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		dev_err(ctx->dev, "Invalid or missing extcon\n");
+		return PTR_ERR(edev);
+	}
+
+	ctx->extcon = edev;
+	ctx->extcon_nb.notifier_call = sii8620_extcon_notifier;
+	INIT_WORK(&ctx->extcon_wq, sii8620_extcon_work);
+	ret = extcon_register_notifier(edev, EXTCON_DISP_MHL, &ctx->extcon_nb);
+	if (ret) {
+		dev_err(ctx->dev, "failed to register notifier for MHL\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static inline struct sii8620 *bridge_to_sii8620(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct sii8620, bridge);
@@ -2201,13 +2279,20 @@ static int sii8620_probe(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	ret = sii8620_extcon_init(ctx);
+	if (ret < 0) {
+		dev_err(ctx->dev, "failed to initialize EXTCON\n");
+		return ret;
+	}
+
 	i2c_set_clientdata(client, ctx);
 
 	ctx->bridge.funcs = &sii8620_bridge_funcs;
 	ctx->bridge.of_node = dev->of_node;
 	drm_bridge_add(&ctx->bridge);
 
-	sii8620_cable_in(ctx);
+	if (!ctx->extcon)
+		sii8620_cable_in(ctx);
 
 	return 0;
 }
@@ -2216,9 +2301,16 @@ static int sii8620_remove(struct i2c_client *client)
 {
 	struct sii8620 *ctx = i2c_get_clientdata(client);
 
-	disable_irq(to_i2c_client(ctx->dev)->irq);
+	if (ctx->extcon) {
+		extcon_unregister_notifier(ctx->extcon, EXTCON_DISP_MHL,
+					   &ctx->extcon_nb);
+		flush_work(&ctx->extcon_wq);
+		if (ctx->cable_state > 0)
+			sii8620_cable_out(ctx);
+	} else {
+		sii8620_cable_out(ctx);
+	}
 	drm_bridge_remove(&ctx->bridge);
-	sii8620_hw_off(ctx);
 
 	return 0;
 }
-- 
2.14.1