Re: [PATCH v4 3/3] arm64: dts: ti: k3-am62p: add the USB sub-system

[Roger Quadros <rogerq@kernel.org>]

On 06/02/2024 19:34, Andrew Davis wrote:
> On 2/6/24 6:30 AM, Roger Quadros wrote:
>>
>>
>> On 05/02/2024 19:34, Andrew Davis wrote:
>>> On 2/5/24 7:59 AM, Roger Quadros wrote:
>>>> There are two USB instances available on the am62p5 starter kit. Include
>>>> and enable them for use on the board.
>>>>
>>>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org>
>>>> ---
>>>>
>>>> Notes:
>>>>       Changelog:
>>>>            v4 - no change
>>>>            v3 - no change
>>>>       https://lore.kernel.org/all/20240201120332.4811-4-rogerq@kernel.org/
>>>>            v2:
>>>>       - added USB PHY CTRL node changes here
>>>>       - changed USB wrapper node names to usb@
>>>>       - changed Type-C chip node name to usb-power-control@
>>>>
>>>>    arch/arm64/boot/dts/ti/k3-am62p-main.dtsi   | 46 ++++++++++++++
>>>>    arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi | 10 +++
>>>>    arch/arm64/boot/dts/ti/k3-am62p5-sk.dts     | 67 +++++++++++++++++++++
>>>>    3 files changed, 123 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
>>>> index 4c51bae06b57..17d28390d587 100644
>>>> --- a/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
>>>> +++ b/arch/arm64/boot/dts/ti/k3-am62p-main.dtsi
>>>> @@ -560,6 +560,52 @@ sdhci2: mmc@fa20000 {
>>>>            status = "disabled";
>>>>        };
>>>>    +    usbss0: usb@f900000 {
>>>> +        compatible = "ti,am62-usb";
>>>> +        reg = <0x00 0x0f900000 0x00 0x800>;
>>>> +        clocks = <&k3_clks 161 3>;
>>>> +        clock-names = "ref";
>>>> +        ti,syscon-phy-pll-refclk = <&usb0_phy_ctrl 0x0>;
>>>> +        #address-cells = <2>;
>>>> +        #size-cells = <2>;
>>>> +        power-domains = <&k3_pds 178 TI_SCI_PD_EXCLUSIVE>;
>>>> +        ranges;
>>>> +        status = "disabled";
>>>> +
>>>> +        usb0: usb@31000000 {
>>>> +            compatible = "snps,dwc3";
>>>> +            reg = <0x00 0x31000000 0x00 0x50000>;
>>>> +            interrupts = <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>, /* irq.0 */
>>>> +            <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>; /* irq.0 */
>>>> +            interrupt-names = "host", "peripheral";
>>>> +            maximum-speed = "high-speed";
>>>> +            dr_mode = "otg";
>>>> +        };
>>>> +    };
>>>> +
>>>> +    usbss1: usb@f910000 {
>>>> +        compatible = "ti,am62-usb";
>>>> +        reg = <0x00 0x0f910000 0x00 0x800>;
>>>> +        clocks = <&k3_clks 162 3>;
>>>> +        clock-names = "ref";
>>>> +        ti,syscon-phy-pll-refclk = <&usb1_phy_ctrl 0x0>;
>>>> +        #address-cells = <2>;
>>>> +        #size-cells = <2>;
>>>> +        power-domains = <&k3_pds 179 TI_SCI_PD_EXCLUSIVE>;
>>>> +        ranges;
>>>> +        status = "disabled";
>>>> +
>>>> +        usb1: usb@31100000 {
>>>> +            compatible = "snps,dwc3";
>>>> +            reg = <0x00 0x31100000 0x00 0x50000>;
>>>> +            interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>, /* irq.0 */
>>>> +            <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>; /* irq.0 */
>>>> +            interrupt-names = "host", "peripheral";
>>>> +            maximum-speed = "high-speed";
>>>> +            dr_mode = "otg";
>>>> +        };
>>>> +    };
>>>> +
>>>>        fss: bus@fc00000 {
>>>>            compatible = "simple-bus";
>>>>            reg = <0x00 0x0fc00000 0x00 0x70000>;
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi
>>>> index 19f42b39394e..00dd38b02a52 100644
>>>> --- a/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi
>>>> +++ b/arch/arm64/boot/dts/ti/k3-am62p-wakeup.dtsi
>>>> @@ -18,6 +18,16 @@ chipid: chipid@14 {
>>>>                reg = <0x14 0x4>;
>>>>                bootph-all;
>>>>            };
>>>> +
>>>> +        usb0_phy_ctrl: syscon@4008 {
>>>> +            compatible = "ti,am62-usb-phy-ctrl", "syscon";
>>>> +            reg = <0x4008 0x4>;
>>>> +        };
>>>> +
>>>> +        usb1_phy_ctrl: syscon@4018 {
>>>> +            compatible = "ti,am62-usb-phy-ctrl", "syscon";
>>>> +            reg = <0x4018 0x4>;
>>>> +        };
>>>>        };
>>>>          wkup_uart0: serial@2b300000 {
>>>> diff --git a/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts b/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
>>>> index 1773c05f752c..80be56c0a4e0 100644
>>>> --- a/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
>>>> +++ b/arch/arm64/boot/dts/ti/k3-am62p5-sk.dts
>>>> @@ -27,6 +27,8 @@ aliases {
>>>>            spi0 = &ospi0;
>>>>            ethernet0 = &cpsw_port1;
>>>>            ethernet1 = &cpsw_port2;
>>>> +        usb0 = &usb0;
>>>> +        usb1 = &usb1;
>>>>        };
>>>>          chosen {
>>>> @@ -297,6 +299,12 @@ AM62PX_IOPAD(0x01b0, PIN_OUTPUT, 2) /* (G20) MCASP0_ACLKR.UART1_TXD */
>>>>            bootph-all;
>>>>        };
>>>>    +    main_usb1_pins_default: main-usb1-default-pins {
>>>> +        pinctrl-single,pins = <
>>>> +            AM62PX_IOPAD(0x0258, PIN_INPUT, 0) /* (G21) USB1_DRVVBUS */
>>>> +        >;
>>>> +    };
>>>> +
>>>>        main_wlirq_pins_default: main-wlirq-default-pins {
>>>>            pinctrl-single,pins = <
>>>>                AM62PX_IOPAD(0x0128, PIN_INPUT, 7) /* (K25) MMC2_SDWP.GPIO0_72 */
>>>> @@ -340,6 +348,36 @@ AM62PX_IOPAD(0x0124, PIN_INPUT, 7) /* (J25) MMC2_SDCD.GPIO0_71 */
>>>>        };
>>>>    };
>>>>    +&main_i2c0 {
>>>> +    status = "okay";
>>>> +    pinctrl-names = "default";
>>>> +    pinctrl-0 = <&main_i2c0_pins_default>;
>>>> +    clock-frequency = <400000>;
>>>> +
>>>> +    typec_pd0: usb-power-controller@3f {
>>>> +        compatible = "ti,tps6598x";
>>>> +        reg = <0x3f>;
>>>> +
>>>> +        connector {
>>>> +            compatible = "usb-c-connector";
>>>> +            label = "USB-C";
>>>> +            self-powered;
>>>> +            data-role = "dual";
>>>> +            power-role = "sink";
>>>> +            ports {
>>>> +                #address-cells = <1>;
>>>> +                #size-cells = <0>;
>>>> +                port@0 {
>>>> +                    reg = <0>;
>>>> +                    usb_con_hs: endpoint {
>>>> +                        remote-endpoint = <&usb0_hs_ep>;
>>>> +                    };
>>>> +                };
>>>> +            };
>>>> +        };
>>>> +    };
>>>> +};
>>>> +
>>>>    &main_i2c1 {
>>>>        status = "okay";
>>>>        pinctrl-names = "default";
>>>> @@ -460,6 +498,35 @@ cpsw3g_phy1: ethernet-phy@1 {
>>>>        };
>>>>    };
>>>>    +&usbss0 {
>>>> +    status = "okay";
>>>> +    ti,vbus-divider;
>>>> +};
>>>> +
>>>> +&usbss1 {
>>>> +    status = "okay";
>>>> +    ti,vbus-divider;
>>>> +};
>>>> +
>>>> +&usb0 {
>>>> +    usb-role-switch;
>>>> +    #address-cells = <1>;
>>>> +    #size-cells = <0>;
>>>> +
>>>> +    port@0 {
>>>> +        reg = <0>;
>>>> +        usb0_hs_ep: endpoint {
>>>> +            remote-endpoint = <&usb_con_hs>;
>>>> +        };
>>>> +    };
>>>> +};
>>>> +
>>>> +&usb1 {
>>>> +    dr_mode = "host";
>>>> +    pinctrl-names = "default";
>>>> +    pinctrl-0 = <&main_usb1_pins_default>;
>>>
>>> I'm not super familiar with USB, but I see this pinmux for the
>>> "DRVVBUS" pin is usually added the the parent USB subsystem node (usbss).
>>> Does this pin belong to the subsystem or the specific USB instance?
>>>
>> There is only 1 USB instance per USB sub-system.
>> The "DRVVBUS" pin is a control signal to enable the VBUS regulator
>> while in host mode.
>>
>> Just probing the usbss1 driver has no use of "DRVVBUS".
>> I think usb1 is the right place as it is used there.
>>
> 
> Seems reasonable, so if there ever was more than 1 instance
> per sub-system, each instance would need its own pin?

Yes.

> 
> We should move these pinctrl to the instance node in the other
> dts files at some point then.
> 
> Reviewed-by: Andrew Davis <afd@ti.com>

Thanks!

> 
>>> Andrew
>>>
>>>> +};
>>>> +
>>>>    &mcasp1 {
>>>>        status = "okay";
>>>>        #sound-dai-cells = <0>;
>>

-- 
cheers,
-roger