Received-SPF: pass (google.com: domain of linux-kernel+bounces-55395-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <8f9e8ec5-fc78-4b51-b36f-3ba06768ce62@ti.com>
Date: Tue, 6 Feb 2024 11:34:48 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 3/3] arm64: dts: ti: k3-am62p: add the USB sub-system
Content-Language: en-US
To: Roger Quadros <rogerq@kernel.org>, <nm@ti.com>, <vigneshr@ti.com>
CC: <kristo@kernel.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>, <conor+dt@kernel.org>,
        <srk@ti.com>, <r-gunasekaran@ti.com>, <b-liu@ti.com>,
        <linux-arm-kernel@lists.infradead.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
References: <20240205135908.54656-1-rogerq@kernel.org>
 <20240205135908.54656-4-rogerq@kernel.org>
 <45a0ed98-8dd2-4c5b-8e89-40c70e3fe831@ti.com>
 <77f7b127-e609-45e3-90aa-67aa7838ce6b@kernel.org>
From: Andrew Davis <afd@ti.com>
In-Reply-To: <77f7b127-e609-45e3-90aa-67aa7838ce6b@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit

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?

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>

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