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

[Eugen Hristev]

On 3/9/23 11:11, Xavier Drudis Ferran wrote:
> El Wed, Mar 08, 2023 at 01:59:54PM +0200, Eugen Hristev deia:
>> 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
>>
> 
> Well, at least if it is always-on for whatever reason, then it is not an
> error that it cannot be turned off.
> 
>>> 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.
>>
> 
> I don't know enough to tell.  I've just looked a little and it seems
> to be used for USB only (on rock pi 4, firefly, eaidk-610,
> khadas-edge, leez-p710, nanopc-t4, orangepi, puma, rock960, rockpro64)
> 
> Curiously rk3399-evb does NOT have regulator-always-on in vcc5v0_host
> 
> and roc-pc seems to add it in u-boot.dtsi only, since it was preserved
> at some u-boot - linux sync.
> 
> pinebook-pro has regulator-always-on, but then has
> regulator-state-mem, regulator-off-in-suspend...
> 
>>
>> Anyway, maybe we should move on even if we can't disable the regulator in
>> any case ? We should just dev_err and continue ?
>>
> 
> dev_err or not dev_err depends on whether always-on is always a bug
> there or may be a feature, I don't know. But moving on would be nice, yes.

Have you tested your case with

if (ret && ret != -EACCES ) {

and it solves your usb reset problem ?

> 
>> Kever, do you have any preference ?
>>
>> Eugen
> 
> Thanks