Re: [PATCH] phy: qcom-snps: Add runtime suspend and resume handlers

[Wesley Cheng <wcheng@codeaurora.org>]

On 4/27/2020 9:59 AM, Vinod Koul wrote:
> On 23-04-20, 10:26, Wesley Cheng wrote:
> 
>> +static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
>> +{
>> +	if (hsphy->suspended)
>> +		return 0;
>> +
>> +	dev_dbg(&hsphy->phy->dev, "Suspend QCOM SNPS PHY, mode = %d \n", hsphy->mode);
>> +
>> +	if (hsphy->mode == PHY_MODE_USB_HOST) {
>> +		/* Enable auto-resume to meet remote wakeup timing */
>> +		qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL2,
>> +										USB2_AUTO_RESUME, USB2_AUTO_RESUME);
>> +		usleep_range(500, 1000);
>> +		qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL2,
>> +										0, USB2_AUTO_RESUME);
> 
> Kernel has a coding guideline where we try to "stick" to 80 char limit
> and is sometimes okay like debug logs. Above is not okay. Please fix it
> and run ./scripts/checkpatch.pl --strict on your patch and fix all
> errors. Warning and checks at your discretion using common sense. When
> in doubt do ask :)
> 

Hi Vinod,

Thanks for the input.  I do run the checkpatch script before sending
patches, and addressing the errors.  However, seems this was tagged as a
warning instead.  Will keep in mind to address as many warnings as I can
as well.

>> +	}
>> +
>> +	clk_disable_unprepare(hsphy->cfg_ahb_clk);
>> +	hsphy->suspended = true;
> 
> why do you need to track this?
> 

More for debug purposes in case the RPM state becomes out of sync with
the expected PHY state.  We've seen some situations during PM
suspend/resume testing where our RPM routines aren't executed, as PM
will disable RPM briefly.  It would be nice to be able to catch these
situations after collecting our crash dumps.

>> +
>> +	return 0;
>> +}
>> +
>> +static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
>> +{
>> +	int ret = 0;
> 
> superfluous init..
> 

Agreed.

>>  static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> @@ -251,6 +333,14 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>>  		return ret;
>>  	}
>>  
>> +	pm_runtime_set_active(dev);
>> +	pm_runtime_enable(dev);
> 
> would it not make sense to enable this after pjy in initialized?
> 

Not sure we want to put this in the phy_init() callback, as we can't
guarantee how the driver registering the PHY will use it. It'll put the
requirement of having to call phy_exit() for every phy_init() sequence,
in order to avoid unbalanced disable_depth warnings from the RPM driver.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project