Received-SPF: pass (google.com: domain of linux-kernel+bounces-63179-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <6429a878-af3c-4182-b65f-60e1c0afccc5@collabora.com>
Date: Tue, 13 Feb 2024 10:09:05 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] arm64: dts: mediatek: mt8186: Add missing clocks to ssusb
 power domains
Content-Language: en-US
To: =?UTF-8?B?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= <nfraprado@collabora.com>,
 Matthias Brugger <matthias.bgg@gmail.com>
Cc: Chen-Yu Tsai <wenst@chromium.org>,
 Eugen Hristev <eugen.hristev@collabora.com>, Rob Herring <robh@kernel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
 Conor Dooley <conor+dt@kernel.org>,
 Allen-KH Cheng <allen-kh.cheng@mediatek.com>, kernel@collabora.com,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org
References: <20240212-mt8186-ssusb-domain-clk-fix-v1-1-26cb98746aa3@collabora.com>
 <985e1a4f-c538-4a9d-be58-2c688aa7350c@notapiano>
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
In-Reply-To: <985e1a4f-c538-4a9d-be58-2c688aa7350c@notapiano>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Il 12/02/24 22:53, Nícolas F. R. A. Prado ha scritto:
> On Mon, Feb 12, 2024 at 04:32:44PM -0500, Nícolas F. R. A. Prado wrote:
>> The ssusb power domains currently don't list any clocks, despite
>> depending on some, and thus rely on the bootloader leaving the required
>> clocks on in order to work.
>>
>> When booting with the upstream arm64 defconfig, the power domain
>> controller will defer probe until modules have loaded since it has an
>> indirect dependency on CONFIG_MTK_CMDQ, which is configured as a module.
>> However at the point where modules are loaded, unused clocks are also
>> disabled, causing the ssusb domains to fail to be enabled and
>> consequently the controller to fail probe:
>>
>> mtk-power-controller 10006000.syscon:power-controller: /soc/syscon@10006000/power-controller/power-domain@4: failed to power on domain: -110
>> mtk-power-controller: probe of 10006000.syscon:power-controller failed with error -110
>>
>> Add the missing clocks to the ssusb power domains so the power
>> controller can boot without relying on bootloader state.
>>
>> Fixes: d9e43c1e7a38 ("arm64: dts: mt8186: Add power domains controller")
>> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
>> ---
>>   arch/arm64/boot/dts/mediatek/mt8186.dtsi | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/mediatek/mt8186.dtsi b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
>> index adaf5e57fac5..02f33ec3cbd3 100644
>> --- a/arch/arm64/boot/dts/mediatek/mt8186.dtsi
>> +++ b/arch/arm64/boot/dts/mediatek/mt8186.dtsi
>> @@ -931,11 +931,19 @@ power-domain@MT8186_POWER_DOMAIN_CSIRX_TOP {
>>   
>>   				power-domain@MT8186_POWER_DOMAIN_SSUSB {
>>   					reg = <MT8186_POWER_DOMAIN_SSUSB>;
>> +					clocks = <&topckgen CLK_TOP_USB_TOP>,
>> +						 <&infracfg_ao CLK_INFRA_AO_SSUSB_TOP_REF>,
>> +						 <&infracfg_ao CLK_INFRA_AO_SSUSB_TOP_XHCI>;
>> +					clock-names = "sys_ck", "ref_ck", "xhci_ck";
>>   					#power-domain-cells = <0>;
>>   				};
>>   
>>   				power-domain@MT8186_POWER_DOMAIN_SSUSB_P1 {
>>   					reg = <MT8186_POWER_DOMAIN_SSUSB_P1>;
>> +					clocks = <&infracfg_ao CLK_INFRA_AO_SSUSB_TOP_P1_SYS>,
>> +						 <&infracfg_ao CLK_INFRA_AO_SSUSB_TOP_P1_REF>,
>> +						 <&infracfg_ao CLK_INFRA_AO_SSUSB_TOP_P1_XHCI>;
>> +					clock-names = "sys_ck", "ref_ck", "xhci_ck";
> 
> I forgot to mention this here, but the XHCI clock wasn't needed to get the power
> domains to work per se, but leaving it out caused issues when probing the mtu3
> devices:
> <3>[   15.431506] mtu3 11201000.usb: clks of sts1 are not stable!
> <3>[   15.443965] mtu3 11201000.usb: device enable failed -110
> <3>[   15.454306] mtu3 11201000.usb: mtu3 hw init failed:-110
> <3>[   15.463865] mtu3 11201000.usb: failed to initialize gadget
> <4>[   15.477890] mtu3: probe of 11201000.usb failed with error -110
> 
> <3>[   15.514603] mtu3 11281000.usb: clks of sts1 are not stable!
> <3>[   15.525239] mtu3 11281000.usb: device enable failed -110
> <3>[   15.614174] mtu3 11281000.usb: mtu3 hw init failed:-110
> <3>[   15.619647] mtu3 11281000.usb: failed to initialize gadget
> <4>[   15.630623] mtu3: probe of 11281000.usb failed with error -110
> 
> Not sure if this issue should be handled separately (maybe the mtu3 device
> should enable the XHCI clock?), but I opted to include the clock here to get
> boot working for this device at once.
> 

Hey Nicolas,
As you just said: having the XHCI clock in the power domain is wrong :-)

Almost comically, the MTU3 binding already supports having a XHCI clock
named "xhci_ck" after "dma_ck"... so the solution is to add the TOP_XHCI
clock in the mtu3 node and that's it. Do not remove it from the children.

mtu3-node-at-somewhere {
		clocks = <&topckgen CLK_TOP_USB_TOP>,
			 <&infracfg_ao CLK_INFRA_AO_SSUSB_TOP_REF>,
			 <&infracfg_ao CLK_INFRA_AO_SSUSB_TOP_HCLK>,
			 <&infracfg_ao CLK_INFRA_AO_ICUSB>,
			 <&infracfg_ao CLK_INFRA_AO_SSUSB_TOP_XHCI>;
		clock-names = "sys_ck", "ref_ck", "mcu_ck", "dma_ck", "xhci_ck";
}

Waiting for a v2...

Cheers,
Angelo