Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
References: <20231002141020.2403652-1-jbrunet@baylibre.com>
 <20231002141020.2403652-3-jbrunet@baylibre.com>
 <b81a296d-0640-4b2e-aab6-c9de37d10206@linaro.org>
 <1j5y3ozvmk.fsf@starbuckisacylon.baylibre.com>
 <3e69ae4b-5d9e-42ee-a21e-151de8fbb996@linaro.org>
User-agent: mu4e 1.8.13; emacs 29.1
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Neil Armstrong <neil.armstrong@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>
Cc:     Kevin Hilman <khilman@baylibre.com>, Da Xue <da.xue@libretech.co>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-amlogic@lists.infradead.org
Subject: Re: [PATCH 2/2] arm64: dts: amlogic: add libretech cottonwood support
Date:   Thu, 05 Oct 2023 11:42:57 +0200
In-reply-to: <3e69ae4b-5d9e-42ee-a21e-151de8fbb996@linaro.org>
Message-ID: <1j8r8hxutt.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk


On Tue 03 Oct 2023 at 09:35, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> On 02/10/2023 20:57, Jerome Brunet wrote:
>> On Mon 02 Oct 2023 at 18:45, Neil Armstrong <neil.armstrong@linaro.org>
>> wrote:
>> 
>
> <snip>
>
>>>> +&usb3_pcie_phy {
>>>> +	#address-cells = <1>;
>>>> +	#size-cells = <0>;
>>>> +	phy-supply = <&vcc_5v>;
>>>> +
>>>> +	hub: hub@1 {
>>>> +		compatible = "usb5e3,626";
>>>> +		reg = <1>;
>>>> +		reset-gpios = <&gpio GPIOC_7 (GPIO_ACTIVE_LOW | GPIO_OPEN_DRAIN)>;
>>>> +	};
>>>
>>> Not sure the PHY is the right place to put the USB HUB,
>>> and it's probable the HUB is connected to both the USB2 and USB3 lines
>> It is connected to the USB3.0 only
>> 
>>> so you should have both USB IDs in DT like it'd done for the Odroid-C4:
>>>
>>> / {
>>> ...
>>>           /* USB hub supports both USB 2.0 and USB 3.0 root hub */
>>>           usb-hub {
>>>                   dr_mode = "host";
>>>                   #address-cells = <1>;
>>>                   #size-cells = <0>;
>>>
>>>                   /* 2.0 hub on port 1 */
>>>                   hub_2_0: hub@1 {
>>>                           compatible = "usb2109,2817";
>>>                           reg = <1>;
>>>                           peer-hub = <&hub_3_0>;
>>>                           reset-gpios = <&gpio GPIOH_4 GPIO_ACTIVE_LOW>;
>>>                           vdd-supply = <&vcc_5v>;
>>>                   };
>>>
>>>                   /* 3.1 hub on port 4 */
>>>                   hub_3_0: hub@2 {
>>>                           compatible = "usb2109,817";
>>>                           reg = <2>;
>>>                           peer-hub = <&hub_2_0>;
>>>                           reset-gpios = <&gpio GPIOH_4 GPIO_ACTIVE_LOW>;
>>>                           vdd-supply = <&vcc_5v>;
>>>                   };
>>>           };
>>> ...
>>> };
>>>
>>> if it only has a single USB ID, then it should go under the dwc3 node.
>> The usb controller is connected to the PHY and what's coming out of the
>> PHY
>> goes to the hub. It seems logical to hub the hub under it.
>> Why bypass the PHY ?
>
> The USB bindings the USB devices nodes should be under the controller's node,
> not the PHY, see:
>
> Documentation/devicetree/bindings/usb/usb-hcd.yaml
> ...
> patternProperties:
>   "^.*@[0-9a-f]{1,2}$":
>     description: The hard wired USB devices
>     type: object
>     $ref: /schemas/usb/usb-device.yaml
> ...
> and the example.
>
> Subnodes aren't allowed in the PHY node.

Ok, that is what schema says.
HW wise there is possible problem though.

The phy node has the power supply to the bus.
In that case it is a controllable one.

If fixed USB devices go under the controller instead of the PHY, isn't
it possible that the kernel may attempt to probe them before the bus is
powered ? For this particular board, it would make the reset we are
trying to apply useless.

>
> Neil
>
>> 
>>>
>>>> +};
>>>> +
>>>> +&usb {
>>>> +	status = "okay";
>>>> +};
>
> <snip>