linux-kernel - Re: [PATCH 2/4] phy: rockchip-usb: use rockchip_usb_phy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <57C40147.60700@ti.com>
Date:   Mon, 29 Aug 2016 15:02:55 +0530
From:   Kishon Vijay Abraham I <kishon@...com>
To:     Heiko Stübner <heiko@...ech.de>
CC:     Randy Li <ayaka@...lik.info>, <linux-usb@...r.kernel.org>,
        <johnyoun@...opsys.com>, <gregkh@...uxfoundation.org>,
        <robh+dt@...nel.org>, <mark.rutland@....com>,
        <devicetree@...r.kernel.org>, <eddie.cai@...k-chips.com>,
        <randy.li@...k-chips.com>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/4] phy: rockchip-usb: use rockchip_usb_phy_reset to
 reset phy during wakeup

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@...lik.info>
>>> ---
>>>
>>>  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