public inbox for linux-usb@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] dt-bindings: usb: Add binding for Microchip usb5744 hub controller
@ 2023-05-03 13:39 Michal Simek
  2023-05-04  6:46 ` Krzysztof Kozlowski
  0 siblings, 1 reply; 6+ messages in thread
From: Michal Simek @ 2023-05-03 13:39 UTC (permalink / raw)
  To: linux-kernel, monstr, michal.simek, git, ilias.apalodimas
  Cc: Greg Kroah-Hartman, Krzysztof Kozlowski, Piyush Mehta,
	Rob Herring, devicetree, linux-usb

The Microchip usb5744 is a SS/HS USB 3.0 hub controller with 4 ports.
The binding describes USB related aspects of the USB5744 hub, it as
well cover the option of connecting the controller as an i2c slave.
When i2c interface is connected hub needs to be initialized first.
Hub itself has fixed i2c address 0x2D but hardcoding address is not good
idea because address can be shifted by i2c address translator in the
middle.

Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
Signed-off-by: Michal Simek <michal.simek@amd.com>
---

It looks like that usb8041 has also an optional i2c interface which is not
covered. But it is mentioned at commit 40e58a8a7ca6 ("dt-bindings: usb:
Add binding for TI USB8041 hub controller").

i2c-bus name property was suggested by Rob at
https://lore.kernel.org/all/CAL_JsqJedhX6typpUKbnzV7CLK6UZVjq3CyG9iY_j5DLPqvVdw@mail.gmail.com/
and
https://lore.kernel.org/all/CAL_JsqJZBbu+UXqUNdZwg-uv0PAsNg55026PTwhKr5wQtxCjVQ@mail.gmail.com/

the question is if adding address like this is acceptable.
But it must be specified.

Driver will follow based on final dt-binding.

---
 .../bindings/usb/microchip,usb5744.yaml       | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/microchip,usb5744.yaml

diff --git a/Documentation/devicetree/bindings/usb/microchip,usb5744.yaml b/Documentation/devicetree/bindings/usb/microchip,usb5744.yaml
new file mode 100644
index 000000000000..fafe275a35df
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/microchip,usb5744.yaml
@@ -0,0 +1,85 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/microchip,usb5744.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip USB5744 4-port Hub Controller
+
+description:
+  Microchip's USB5744 SmartHubTM IC is a 4 port, SuperSpeed (SS)/Hi-Speed (HS),
+  low power, low pin count configurable and fully compliant with the USB 3.1
+  Gen 1 specification. The USB5744 also supports Full Speed (FS) and Low Speed
+  (LS) USB signaling, offering complete coverage of all defined USB operating
+  speeds. The new SuperSpeed hubs operate in parallel with the USB 2.0
+  controller, so 5 Gbps SuperSpeed data transfers are not affected by slower
+  USB 2.0 traffic.
+
+maintainers:
+  - Piyush Mehta <piyush.mehta@amd.com>
+  - Michal Simek <michal.simek@amd.com>
+
+allOf:
+  - $ref: usb-device.yaml#
+
+properties:
+  compatible:
+    enum:
+      - usb424,5744
+      - usb424,2744
+
+  reg: true
+
+  reset-gpios:
+    maxItems: 1
+    description:
+      GPIO controlling the GRST# pin.
+
+  vdd-supply:
+    description:
+      VDD power supply to the hub
+
+  peer-hub:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      phandle to the peer hub on the controller.
+
+  i2c-bus:
+    maxItems: 1
+    description:
+      phandle of an I2C controller to link usb-hub for usb attach and reset
+      followed by i2c address.
+
+required:
+  - compatible
+  - reg
+  - peer-hub
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    usb {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        /* 2.0 hub on port 1 */
+        hub_2_0: hub@1 {
+          compatible = "usb424,5744";
+          reg = <1>;
+          peer-hub = <&hub_3_0>;
+          i2c-bus = <&i2c 0x2d>;
+          reset-gpios = <&gpio 3 GPIO_ACTIVE_LOW>;
+        };
+
+        /* 3.0 hub on port 2 */
+        hub_3_0: hub@2 {
+          compatible = "usb424,2744";
+          reg = <2>;
+          peer-hub = <&hub_2_0>;
+          i2c-bus = <&i2c 0x2d>;
+          reset-gpios = <&gpio 3 GPIO_ACTIVE_LOW>;
+        };
+    };
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] dt-bindings: usb: Add binding for Microchip usb5744 hub controller
  2023-05-03 13:39 [PATCH] dt-bindings: usb: Add binding for Microchip usb5744 hub controller Michal Simek
@ 2023-05-04  6:46 ` Krzysztof Kozlowski
  2023-05-04  7:25   ` Michal Simek
  0 siblings, 1 reply; 6+ messages in thread
From: Krzysztof Kozlowski @ 2023-05-04  6:46 UTC (permalink / raw)
  To: Michal Simek, linux-kernel, monstr, michal.simek, git,
	ilias.apalodimas
  Cc: Greg Kroah-Hartman, Krzysztof Kozlowski, Piyush Mehta,
	Rob Herring, devicetree, linux-usb

On 03/05/2023 15:39, Michal Simek wrote:
> The Microchip usb5744 is a SS/HS USB 3.0 hub controller with 4 ports.
> The binding describes USB related aspects of the USB5744 hub, it as
> well cover the option of connecting the controller as an i2c slave.
> When i2c interface is connected hub needs to be initialized first.
> Hub itself has fixed i2c address 0x2D but hardcoding address is not good
> idea because address can be shifted by i2c address translator in the
> middle.
> 
> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
> Signed-off-by: Michal Simek <michal.simek@amd.com>
> ---
> 
> It looks like that usb8041 has also an optional i2c interface which is not
> covered. But it is mentioned at commit 40e58a8a7ca6 ("dt-bindings: usb:
> Add binding for TI USB8041 hub controller").
> 
> i2c-bus name property was suggested by Rob at
> https://lore.kernel.org/all/CAL_JsqJedhX6typpUKbnzV7CLK6UZVjq3CyG9iY_j5DLPqvVdw@mail.gmail.com/
> and
> https://lore.kernel.org/all/CAL_JsqJZBbu+UXqUNdZwg-uv0PAsNg55026PTwhKr5wQtxCjVQ@mail.gmail.com/
> 
> the question is if adding address like this is acceptable.
> But it must be specified.

Why? phandle points it explicitly.

> 
> Driver will follow based on final dt-binding.
> 
> ---
>  .../bindings/usb/microchip,usb5744.yaml       | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/microchip,usb5744.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/microchip,usb5744.yaml b/Documentation/devicetree/bindings/usb/microchip,usb5744.yaml
> new file mode 100644
> index 000000000000..fafe275a35df
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/microchip,usb5744.yaml
> @@ -0,0 +1,85 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/microchip,usb5744.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip USB5744 4-port Hub Controller
> +
> +description:
> +  Microchip's USB5744 SmartHubTM IC is a 4 port, SuperSpeed (SS)/Hi-Speed (HS),
> +  low power, low pin count configurable and fully compliant with the USB 3.1
> +  Gen 1 specification. The USB5744 also supports Full Speed (FS) and Low Speed
> +  (LS) USB signaling, offering complete coverage of all defined USB operating
> +  speeds. The new SuperSpeed hubs operate in parallel with the USB 2.0
> +  controller, so 5 Gbps SuperSpeed data transfers are not affected by slower
> +  USB 2.0 traffic.
> +
> +maintainers:
> +  - Piyush Mehta <piyush.mehta@amd.com>
> +  - Michal Simek <michal.simek@amd.com>
> +
> +allOf:
> +  - $ref: usb-device.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - usb424,5744
> +      - usb424,2744

Keep the list ordered, so 2744 before 5744.

> +
> +  reg: true
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description:
> +      GPIO controlling the GRST# pin.
> +
> +  vdd-supply:
> +    description:
> +      VDD power supply to the hub
> +
> +  peer-hub:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      phandle to the peer hub on the controller.
> +
> +  i2c-bus:
> +    maxItems: 1
> +    description:
> +      phandle of an I2C controller to link usb-hub for usb attach and reset
> +      followed by i2c address.
> +
> +required:
> +  - compatible
> +  - reg
> +  - peer-hub
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    usb {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        /* 2.0 hub on port 1 */
> +        hub_2_0: hub@1 {
> +          compatible = "usb424,5744";

Mixed indentation, use four spaces here as well.

> +          reg = <1>;
> +          peer-hub = <&hub_3_0>;
> +          i2c-bus = <&i2c 0x2d>;
> +          reset-gpios = <&gpio 3 GPIO_ACTIVE_LOW>;
> +        };
Krzysztof


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] dt-bindings: usb: Add binding for Microchip usb5744 hub controller
  2023-05-04  6:46 ` Krzysztof Kozlowski
@ 2023-05-04  7:25   ` Michal Simek
  2023-05-04  7:31     ` Krzysztof Kozlowski
  0 siblings, 1 reply; 6+ messages in thread
From: Michal Simek @ 2023-05-04  7:25 UTC (permalink / raw)
  To: Krzysztof Kozlowski, linux-kernel, monstr, michal.simek, git,
	ilias.apalodimas
  Cc: Greg Kroah-Hartman, Krzysztof Kozlowski, Piyush Mehta,
	Rob Herring, devicetree, linux-usb



On 5/4/23 08:46, Krzysztof Kozlowski wrote:
> On 03/05/2023 15:39, Michal Simek wrote:
>> The Microchip usb5744 is a SS/HS USB 3.0 hub controller with 4 ports.
>> The binding describes USB related aspects of the USB5744 hub, it as
>> well cover the option of connecting the controller as an i2c slave.
>> When i2c interface is connected hub needs to be initialized first.
>> Hub itself has fixed i2c address 0x2D but hardcoding address is not good
>> idea because address can be shifted by i2c address translator in the
>> middle.
>>
>> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>> ---
>>
>> It looks like that usb8041 has also an optional i2c interface which is not
>> covered. But it is mentioned at commit 40e58a8a7ca6 ("dt-bindings: usb:
>> Add binding for TI USB8041 hub controller").
>>
>> i2c-bus name property was suggested by Rob at
>> https://lore.kernel.org/all/CAL_JsqJedhX6typpUKbnzV7CLK6UZVjq3CyG9iY_j5DLPqvVdw@mail.gmail.com/
>> and
>> https://lore.kernel.org/all/CAL_JsqJZBbu+UXqUNdZwg-uv0PAsNg55026PTwhKr5wQtxCjVQ@mail.gmail.com/
>>
>> the question is if adding address like this is acceptable.
>> But it must be specified.
> 
> Why? phandle points it explicitly.

Ok it means just list usb hub on i2c with label and point to it. Works for me.

Thanks,
Michal

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] dt-bindings: usb: Add binding for Microchip usb5744 hub controller
  2023-05-04  7:25   ` Michal Simek
@ 2023-05-04  7:31     ` Krzysztof Kozlowski
  2023-05-04  7:55       ` Michal Simek
  0 siblings, 1 reply; 6+ messages in thread
From: Krzysztof Kozlowski @ 2023-05-04  7:31 UTC (permalink / raw)
  To: Michal Simek, linux-kernel, monstr, michal.simek, git,
	ilias.apalodimas
  Cc: Greg Kroah-Hartman, Krzysztof Kozlowski, Piyush Mehta,
	Rob Herring, devicetree, linux-usb

On 04/05/2023 09:25, Michal Simek wrote:
> 
> 
> On 5/4/23 08:46, Krzysztof Kozlowski wrote:
>> On 03/05/2023 15:39, Michal Simek wrote:
>>> The Microchip usb5744 is a SS/HS USB 3.0 hub controller with 4 ports.
>>> The binding describes USB related aspects of the USB5744 hub, it as
>>> well cover the option of connecting the controller as an i2c slave.
>>> When i2c interface is connected hub needs to be initialized first.
>>> Hub itself has fixed i2c address 0x2D but hardcoding address is not good
>>> idea because address can be shifted by i2c address translator in the
>>> middle.
>>>
>>> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>> ---
>>>
>>> It looks like that usb8041 has also an optional i2c interface which is not
>>> covered. But it is mentioned at commit 40e58a8a7ca6 ("dt-bindings: usb:
>>> Add binding for TI USB8041 hub controller").
>>>
>>> i2c-bus name property was suggested by Rob at
>>> https://lore.kernel.org/all/CAL_JsqJedhX6typpUKbnzV7CLK6UZVjq3CyG9iY_j5DLPqvVdw@mail.gmail.com/
>>> and
>>> https://lore.kernel.org/all/CAL_JsqJZBbu+UXqUNdZwg-uv0PAsNg55026PTwhKr5wQtxCjVQ@mail.gmail.com/
>>>
>>> the question is if adding address like this is acceptable.
>>> But it must be specified.
>>
>> Why? phandle points it explicitly.
> 
> Ok it means just list usb hub on i2c with label and point to it. Works for me.

Right. I missed you want the address of the hub but phandle goes to the
bus. I think listing it on I2C bus (see
arch/arm/boot/dts/vf610-zii-scu4-aib.dts) should work. I think we can
have I2C devices without compatibles.

The problem is that property should have only one definition/type and
i2c-bus is already used in other cases as just "phandle". If we go with
your phandle+address approach, then this should be phandle-array with
items and then we have two different types.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] dt-bindings: usb: Add binding for Microchip usb5744 hub controller
  2023-05-04  7:31     ` Krzysztof Kozlowski
@ 2023-05-04  7:55       ` Michal Simek
  2023-05-04  8:08         ` Krzysztof Kozlowski
  0 siblings, 1 reply; 6+ messages in thread
From: Michal Simek @ 2023-05-04  7:55 UTC (permalink / raw)
  To: Krzysztof Kozlowski, linux-kernel, monstr, michal.simek, git,
	ilias.apalodimas, Marek Vasut
  Cc: Greg Kroah-Hartman, Krzysztof Kozlowski, Piyush Mehta,
	Rob Herring, devicetree, linux-usb



On 5/4/23 09:31, Krzysztof Kozlowski wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> 
> 
> On 04/05/2023 09:25, Michal Simek wrote:
>>
>>
>> On 5/4/23 08:46, Krzysztof Kozlowski wrote:
>>> On 03/05/2023 15:39, Michal Simek wrote:
>>>> The Microchip usb5744 is a SS/HS USB 3.0 hub controller with 4 ports.
>>>> The binding describes USB related aspects of the USB5744 hub, it as
>>>> well cover the option of connecting the controller as an i2c slave.
>>>> When i2c interface is connected hub needs to be initialized first.
>>>> Hub itself has fixed i2c address 0x2D but hardcoding address is not good
>>>> idea because address can be shifted by i2c address translator in the
>>>> middle.
>>>>
>>>> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>>> ---
>>>>
>>>> It looks like that usb8041 has also an optional i2c interface which is not
>>>> covered. But it is mentioned at commit 40e58a8a7ca6 ("dt-bindings: usb:
>>>> Add binding for TI USB8041 hub controller").
>>>>
>>>> i2c-bus name property was suggested by Rob at
>>>> https://lore.kernel.org/all/CAL_JsqJedhX6typpUKbnzV7CLK6UZVjq3CyG9iY_j5DLPqvVdw@mail.gmail.com/
>>>> and
>>>> https://lore.kernel.org/all/CAL_JsqJZBbu+UXqUNdZwg-uv0PAsNg55026PTwhKr5wQtxCjVQ@mail.gmail.com/
>>>>
>>>> the question is if adding address like this is acceptable.
>>>> But it must be specified.
>>>
>>> Why? phandle points it explicitly.
>>
>> Ok it means just list usb hub on i2c with label and point to it. Works for me.
> 
> Right. I missed you want the address of the hub but phandle goes to the
> bus. I think listing it on I2C bus (see
> arch/arm/boot/dts/vf610-zii-scu4-aib.dts) should work. I think we can
> have I2C devices without compatibles.

Device is definitely on i2c bus. But the problem with phande to bus is that 
there could more the same usb hubs and different i2c addresses of it. That's why 
I need to have exact match.
Marek has similar hub where i2c address can be strapped too.

> The problem is that property should have only one definition/type and
> i2c-bus is already used in other cases as just "phandle". If we go with
> your phandle+address approach, then this should be phandle-array with
> items and then we have two different types.

What to do with it then?

Thanks,
Michal

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] dt-bindings: usb: Add binding for Microchip usb5744 hub controller
  2023-05-04  7:55       ` Michal Simek
@ 2023-05-04  8:08         ` Krzysztof Kozlowski
  0 siblings, 0 replies; 6+ messages in thread
From: Krzysztof Kozlowski @ 2023-05-04  8:08 UTC (permalink / raw)
  To: Michal Simek, linux-kernel, monstr, michal.simek, git,
	ilias.apalodimas, Marek Vasut
  Cc: Greg Kroah-Hartman, Krzysztof Kozlowski, Piyush Mehta,
	Rob Herring, devicetree, linux-usb

On 04/05/2023 09:55, Michal Simek wrote:
> 
> 
> On 5/4/23 09:31, Krzysztof Kozlowski wrote:
>> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>>
>>
>> On 04/05/2023 09:25, Michal Simek wrote:
>>>
>>>
>>> On 5/4/23 08:46, Krzysztof Kozlowski wrote:
>>>> On 03/05/2023 15:39, Michal Simek wrote:
>>>>> The Microchip usb5744 is a SS/HS USB 3.0 hub controller with 4 ports.
>>>>> The binding describes USB related aspects of the USB5744 hub, it as
>>>>> well cover the option of connecting the controller as an i2c slave.
>>>>> When i2c interface is connected hub needs to be initialized first.
>>>>> Hub itself has fixed i2c address 0x2D but hardcoding address is not good
>>>>> idea because address can be shifted by i2c address translator in the
>>>>> middle.
>>>>>
>>>>> Signed-off-by: Piyush Mehta <piyush.mehta@amd.com>
>>>>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>>>>> ---
>>>>>
>>>>> It looks like that usb8041 has also an optional i2c interface which is not
>>>>> covered. But it is mentioned at commit 40e58a8a7ca6 ("dt-bindings: usb:
>>>>> Add binding for TI USB8041 hub controller").
>>>>>
>>>>> i2c-bus name property was suggested by Rob at
>>>>> https://lore.kernel.org/all/CAL_JsqJedhX6typpUKbnzV7CLK6UZVjq3CyG9iY_j5DLPqvVdw@mail.gmail.com/
>>>>> and
>>>>> https://lore.kernel.org/all/CAL_JsqJZBbu+UXqUNdZwg-uv0PAsNg55026PTwhKr5wQtxCjVQ@mail.gmail.com/
>>>>>
>>>>> the question is if adding address like this is acceptable.
>>>>> But it must be specified.
>>>>
>>>> Why? phandle points it explicitly.
>>>
>>> Ok it means just list usb hub on i2c with label and point to it. Works for me.
>>
>> Right. I missed you want the address of the hub but phandle goes to the
>> bus. I think listing it on I2C bus (see
>> arch/arm/boot/dts/vf610-zii-scu4-aib.dts) should work. I think we can
>> have I2C devices without compatibles.
> 
> Device is definitely on i2c bus. But the problem with phande to bus is that 
> there could more the same usb hubs and different i2c addresses of it. That's why 
> I need to have exact match.
> Marek has similar hub where i2c address can be strapped too.
> 
>> The problem is that property should have only one definition/type and
>> i2c-bus is already used in other cases as just "phandle". If we go with
>> your phandle+address approach, then this should be phandle-array with
>> items and then we have two different types.
> 
> What to do with it then?

Your idea. I think you missed part of my comment. Add hub to the I2C bus
and phandle to the hub I2C device node.

Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-05-04  8:10 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-03 13:39 [PATCH] dt-bindings: usb: Add binding for Microchip usb5744 hub controller Michal Simek
2023-05-04  6:46 ` Krzysztof Kozlowski
2023-05-04  7:25   ` Michal Simek
2023-05-04  7:31     ` Krzysztof Kozlowski
2023-05-04  7:55       ` Michal Simek
2023-05-04  8:08         ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox