[PATCH 2/3] phy: rockchip-inno-usb2: add initial support for rk3588 PHY

[Eugen Hristev]

On 3/8/23 13:30, Xavier Drudis Ferran wrote:
> El Fri, Mar 03, 2023 at 09:31:33AM +0200, Eugen Hristev deia:
>> @@ -105,6 +130,17 @@ static int rockchip_usb2phy_power_off(struct phy *phy)
>>   	struct udevice *parent = dev_get_parent(phy->dev);
>>   	struct rockchip_usb2phy *priv = dev_get_priv(parent);
>>   	const struct rockchip_usb2phy_port_cfg *port_cfg = us2phy_get_port(phy);
>> +	struct udevice *vbus = NULL;
>> +	int ret;
>> +
>> +	vbus = rockchip_usb2phy_check_vbus(phy);
>> +	if (vbus) {
>> +		ret = regulator_set_enable(vbus, false);
>> +		if (ret) {
> 
> Could we have
> 	if (ret && ret != -EACCES ) {
> instead here ?
> (reason below)
Hi,

I have nothing against it, the regulator should be all the way optional IMO

> 
>> +			dev_err(phy->dev, "vbus disable failed: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>>   
>>   	property_enable(priv->reg_base, &port_cfg->phy_sus, true);
>>
> 
> Thank you.
> 
> I've tested a little your patch (on an older master, 4eb7c5030d3f3c70
> (2023-02-19) and some minor config changes that don't seem to matter).
> I could try on current master if needed, but I thought I'd ask already.
> 
> In a Rock-pi 4B the ehci ports weren't working without your patch and
> they still don't work with your patch.
> 
> With my patch (v5, [1]) they worked, but with my patch and yours usb reset
> fails:
> 
> resetting USB...
> rockchip_usb2phy_port host-port: vbus disable failed: -13
> rockchip_usb2phy_port host-port: PHY: Failed to power off host-port: -13.
> device_remove: Device 'usb at fe38    ' failed to remove, but children are gone
> rockchip_usb2phy_port host-port: vbus disable failed: -13
> rockchip_usb2phy_port host-port: PHY: Failed to power off host-port: -13.
> device_remove: Device 'usb at fe3c0000' failed to remove, but children are gone
> Bus usb at fe380000: Bus usb at fe3c0000: Bus usb at fe800000: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> Bus usb at fe900000: Register 2000140 NbrPorts 2
> Starting the controller
> USB XHCI 1.10
> scanning bus usb at fe380000 for devices... "Synchronous Abort" handler, esr 0x96000010
> elr: 000000000021de20 lr : 000000000021de20 (reloc)
> elr: 00000000f3f56e20 lr : 00000000f3f56e20
> x0 : 0000000000000000 x1 : 00000000f1f19fef
> x2 : 00000000f1f19fee x3 : 00000000f1f53bc0
> x4 : 0000000000000000 x5 : 0000000000000000
> x6 : 0000000000000000 x7 : 00000000f1f60000
> x8 : 0000000000000100 x9 : 0000000000000008
> x10: 0000000000000006 x11: 0000000000000080
> x12: 000000000001869f x13: 000000000000869c
> x14: 0000000000000000 x15: 0000000000000002
> x16: 000000007e4f2113 x17: 000000009a11f13e
> x18: 00000000f1f30d90 x19: 00000000f1f323c0
> x20: 00000000f1f1a680 x21: 00000000f1f19fee
> x22: 00000000f1f19fef x23: 0000000088404000
> x24: 00000000f1f1a280 x25: 0000000000000001
> x26: 00000000f1f1a680 x27: 00000000f1f1a0c0
> x28: 0000000008400000 x29: 00000000f1f19f90
> 
> Code: aa0203f5 f944a413 aa1303e0 94003d8f (b9400401)
> Resetting CPU ...
> 
> resetting ...
> 
> 
> The apparent reason is that arch/arm/dts/rk3399-rock-pi-4.dtsi
> says
> 
> 	vcc5v0_host: vcc5v0-host-regulator {
> 		compatible = "regulator-fixed";
> 		enable-active-high;
> 		gpio = <&gpio4 RK_PD1 GPIO_ACTIVE_HIGH>;
> 		pinctrl-names = "default";
> 		pinctrl-0 = <&vcc5v0_host_en>;
> 		regulator-name = "vcc5v0_host";
> 
> /*****/ regulator-always-on; /*****/

Pretty weird that a regulator that can be turned on/off via a GPIO is 
'regulator-always-on'. I find this odd and i think it's not correctly 
described at DT level.

> 
> 		vin-supply = <&vcc5v0_sys>;
> 	};
> [...]
> &u2phy0 {
> 	status = "okay";
> 
> 	u2phy0_otg: otg-port {
> 		status = "okay";
> 	};
> 
> 	u2phy0_host: host-port {
> 		phy-supply = <&vcc5v0_host>;
> 		status = "okay";
> 	};
> };
> 
> &u2phy1 {
> 	status = "okay";
> 
> 	u2phy1_otg: otg-port {
> 		status = "okay";
> 	};
> 
> 	u2phy1_host: host-port {
> 		phy-supply = <&vcc5v0_host>;
> 		status = "okay";
> 	};
> };
> 
> 
> and the regulator_set_enable() in regulator-uclass.c when passed enable==0
> will return -EACCES (meaning you can't turn off an always-on regulator).

Anyway, maybe we should move on even if we can't disable the regulator 
in any case ? We should just dev_err and continue ?

Kever, do you have any preference ?

Eugen
> 
> Now my patch is not accepted, so EHCI isn't working on Rock-pi-4, so the
> error won't show.
> 
> But I suspect it may bite if v5 is ever accepted. And I think it would fail if
> EHCI is fixed in any other way. I may try my v1, v2 or v3 with your patch if
> you think it's worth it.
> 
> Now I don't know what the right solution is :
> 
> - ignore -EACCES in rockchip_usb2phy_power_off() (as above, seems easiest?)
> 
> - check always_on in rockchip_usb2phy_power_off() before calling regulator_set_enable()
> 
> - change .dts (but they come from linux, don't they?)
> 
> - make sure somehow that things don't break even if
>    rockchip_usb2phy_power_off() exits early with error.
> 
> Btw, my v5 seems not to be correctly in patchwork, should I resend ?
> I guess I messed up subject lines ?
> 
> I think there are other rk3399 boards that have similar .dtsi files,
> but I haven't investigated thoroughly. Do they all have broken EHCI ?
> If someone has a rk3399 board with working EHCI maybe they should test
> your patch and try usb start ; usb stop and usb start ; usb reset ?
> 
> [1] https://lists.denx.de/pipermail/u-boot/2023-February/510672.html
> 
> Thanks,
> 
> --
> Xavier Drudis Ferran