Re: [PATCH 2/4] phy: rockchip-usb: use rockchip_usb_phy_reset to reset phy during wakeup

[Kishon Vijay Abraham I]

Hi,

On Monday 22 August 2016 06:30 PM, Heiko Stübner wrote:
> Am Montag, 22. August 2016, 17:17:41 schrieb Kishon Vijay Abraham I:
>> Hi,
>>
>> On Sunday 21 August 2016 02:02 AM, Randy Li wrote:
>>> It is a hardware bug in RK3288, the only way to solve it is to
>>> reset the phy.
>>>
>>> Signed-off-by: Randy Li <ayaka@xxxxxxxxxxx>
>>> ---
>>>
>>>  drivers/phy/phy-rockchip-usb.c | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/phy/phy-rockchip-usb.c
>>> b/drivers/phy/phy-rockchip-usb.c index 2a7381f..734987f 100644
>>> --- a/drivers/phy/phy-rockchip-usb.c
>>> +++ b/drivers/phy/phy-rockchip-usb.c
>>> @@ -29,6 +29,7 @@
>>>
>>>  #include <linux/reset.h>
>>>  #include <linux/regmap.h>
>>>  #include <linux/mfd/syscon.h>
>>>
>>> +#include <linux/delay.h>
>>>
>>>  static int enable_usb_uart;
>>>
>>> @@ -64,6 +65,7 @@ struct rockchip_usb_phy {
>>>
>>>  	struct clk_hw	clk480m_hw;
>>>  	struct phy	*phy;
>>>  	bool		uart_enabled;
>>>
>>> +	struct reset_control *reset;
>>>
>>>  };
>>>  
>>>  static int rockchip_usb_phy_power(struct rockchip_usb_phy *phy,
>>>
>>> @@ -144,9 +146,23 @@ static int rockchip_usb_phy_power_on(struct phy
>>> *_phy)
>>>
>>>  	return clk_prepare_enable(phy->clk480m);
>>>  
>>>  }
>>>
>>> +static int rockchip_usb_phy_reset(struct phy *_phy)
>>> +{
>>> +	struct rockchip_usb_phy *phy = phy_get_drvdata(_phy);
>>> +
>>> +	if (phy->reset) {
>>> +		reset_control_assert(phy->reset);
>>> +		udelay(10);
>>> +		reset_control_deassert(phy->reset);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>
>>>  static const struct phy_ops ops = {
>>>  
>>>  	.power_on	= rockchip_usb_phy_power_on,
>>>  	.power_off	= rockchip_usb_phy_power_off,
>>>
>>> +	.reset		= rockchip_usb_phy_reset,
>>
>> why not just reuse the .init ops? reset can be done during initialization
>> right?
> 
> The naming of power_on + power_off and init + exit probably suggests that they 
> are supposed to be used in pairs. (aka module_init + module_exit and probably 
> more)
> 
> But in fact I've seen different combinations so far (phy_init + phy_power_on 
> ... phy_power_off + phy_exit but also phy_power_on + phy_init ... phy_exit + 
> phy_power_off), so I guess the semantics are not that strictly defined.

yeah, it got difficult to maintain the semantics once the number of phy users
increased. Thought it was better than bloating the framework with platform
specific callbacks.

Thanks
Kishon