On Fri, Jan 11, 2013 at 6:00 PM, Subhash Jadavani
<subhashj@xxxxxxxxxxxxxx> wrote:
> On 1/11/2013 4:11 PM, Sujit Reddy Thumma wrote:
>>
>> On 1/9/2013 5:41 PM, vinayak holikatti wrote:
>>>
>>> On Mon, Jan 7, 2013 at 1:11 PM, Sujit Reddy Thumma
>>> <sthumma@xxxxxxxxxxxxxx> wrote:
>>>>
>>>> Hi Vinayak,
>>>>
>>>> I have few comments below:
>>>>
>>>>
>>>>>>>> +#ifdef CONFIG_PM
>>>>>>>> +/**
>>>>>>>> + * ufshcd_pltfrm_suspend - suspend power management function
>>>>>>>> + * @pdev: pointer to Platform device handle
>>>>>>>> + * @mesg: power state
>>>>>>>> + *
>>>>>>>> + * Returns -ENOSYS
>>>>
>>>>
>>>> What breaks if you return 0 instead of return -ENOSYS? Returning error
>>>> seems
>>>> to break platform suspend/resume until all the TODO's are addressed. If
>>>> the
>>>> current s/w cannot make h/w suspend, it should be okay to let the rest
>>>> of
>>>> the system be suspended.
>>>>
>>>
>>> In that case how will the controller be in a working state once it
>>> resumes.
>>> It does not make sense to return zero and to notify the OS that
>>> everything is fine.
>>
>>
>> Since the suspend routine doesn't do anything except returning zero, no
>> power/clocks would be removed and the controller should be in the same state
>> after resume. Do you see any system that removes power/clocks to controllers
>> during suspend without knowledge of corresponding drivers? If so, then such
>> systems must be fixed. In any case, blocking entire system suspend just
>> because s/w isn't taking care of powering down one controller is not a good
>> idea.
>>
>> I would like to hear from others on this as well.
>
>
> Yes, i agree with Sujit that there is no point in blocking the entire system
> suspend just because ufshcd haven't implemented their suspend functionality.
> returning 0 from this function should be fine. And as Sujit already
> mentioned, if during resume you don't find the UFS (controller / phy) state
> as it was left in suspend then it's a particular system's issue and which
> needs to be fixed.
We Implemented Suspend/Resume place holders seeing the kernel documentation
in submitting device drivers
I would like to know if the suggested code change works with any
existing system.
Does any one have similar point of view as Sujit and Subhash have?
>
>>
>>>
>>>>>>>> + */
>>>>>>>> +static int ufshcd_pltfrm_suspend(struct platform_device *pdev,
>>>>>>>> + pm_message_t mesg)
>>>>>>>> +{
>>>>>>>> + /*
>>>>>>>> + * TODO:
>>>>>>>> + * 1. Call ufshcd_suspend
>>>>>>>> + * 2. Do bus specific power management
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> + return -ENOSYS;
>>>>
>>>>
>>>> Returning error doesn't allow entire system to be suspended. Perhaps,
>>>> you
>>>> can do disable_irq() and return 0?
>>>>
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * ufshcd_pltfrm_resume - resume power management function
>>>>>>>> + * @pdev: pointer to Platform device handle
>>>>>>>> + *
>>>>>>>> + * Returns -ENOSYS
>>>>>>>> + */
>>>>>>>> +static int ufshcd_pltfrm_resume(struct platform_device *pdev)
>>>>>>>> +{
>>>>>>>> + /*
>>>>>>>> + * TODO:
>>>>>>>> + * 1. Call ufshcd_resume.
>>>>>>>> + * 2. Do bus specific wake up
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> + return -ENOSYS;
>>>>
>>>>
>>>> enable_irq() and return 0?
>>>>
>>>>>>>> +}
>>>>>>>> +#endif
>>>>>>>> +
>>>>
>>>>
>>>>>>>> +static int __devexit ufshcd_pltfrm_remove(struct platform_device
>>>>>>>> *pdev)
>>>>>>>> +{
>>>>>>>> + struct resource *mem_res;
>>>>>>>> + struct resource *irq_res;
>>>>>>>> + resource_size_t mem_size;
>>>>>>>> + struct ufs_hba *hba = platform_get_drvdata(pdev);
>>>>>>>> +
>>>>>>>> + irq_res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>>>>>>>
>>>>>>>
>>>>>>> It would be better to save the irq number under "struct ufs_hba"
>>>>>>> during
>>>>>>> probe. So here during remove you just need simply need to call the
>>>>>>> "free_irq(irq_res->start, hba)"
>>>>>>
>>>>>>
>>>>>> Will modify the code accordingly in the next patchset.
>>>>>>>>
>>>>>>>>
>>>>>>>> +
>>>>>>>> + if (!irq_res)
>>>>>>>> + dev_err(&pdev->dev, "ufshcd: IRQ resource not
>>>>>>>> available\n");
>>>>>>>> + else
>>>>>>>> + free_irq(irq_res->start, hba);
>>>>
>>>>
>>>>
>>>> The documentation of free_irq says:
>>>> "... On a shared IRQ the caller must ensure the interrupt is disabled on
>>>> the
>>>> card it drives before calling this function. .." I don't see
>>>> disable_irq()
>>>> getting called either here or ufshcd_remove().
>>>
>>>
>>> Why would you want to disable the entire IRQ line when it is shared?
>>> Logical thing is to disable the interrupt on the controller.
>>
>>
>> Since you have enabled the irq in ufshcd_init() and decremented the
>> desc->depth you need to need to do disable_irq(). disable_irq() doesn't
>> disable the irq line until all the shared irq drivers disables it.
>>
>> Also, on some systems not calling disable_irq() could be a problem - the
>> power to wakeup irq monitor block couldn't be turned off if there are active
>> irqs.
>>
>>>
>>>>
>>>>
>>>>>>>> +
>>>>>>>> + ufshcd_remove(hba);
>>>>>>>
>>>>>>>
>>>>>>> Remove should be exactly opposite of probe(). So shouldn't you call
>>>>>>> the
>>>>>>> ufshcd_remove() first and then free_irq() after that.
>>>>>>
>>>>>>
>>>>>> Some bugging controllers might raise the interrupt after resources are
>>>>>> removed.
>>>>>> this sequence will prevent it.
>>>>>
>>>>>
>>>>>
>>>>> Could you please add the same as comment in above code sequence?
>>>>>
>>>>>>>> + mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>>>>>
>>>>>>>
>>>>>>> You might want to save the pointer to mem_res in "struct ufs_hba"
>>>>>>> during
>>>>>>> probe and may use the same here.
>>>>>>>>
>>>>>>>>
>>>>>>>> + if (!mem_res)
>>>>>>>> + dev_err(&pdev->dev, "ufshcd: Memory resource not
>>>>>>>> available\n");
>>>>>>>> + else {
>>>>>>>> + mem_size = resource_size(mem_res);
>>>>>>>> + release_mem_region(mem_res->start, mem_size);
>>>>>>>> + }
>>>>>>>> + platform_set_drvdata(pdev, NULL);
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static const struct of_device_id ufs_of_match[] = {
>>>>>>>> + { .compatible = "jedec,ufs-1.1"},
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static struct platform_driver ufshcd_pltfrm_driver = {
>>>>>>>> + .probe = ufshcd_pltfrm_probe,
>>>>>>>> + .remove = __devexit_p(ufshcd_pltfrm_remove),
>>>>>>>> +#ifdef CONFIG_PM
>>>>
>>>>
>>>>
>>>> CONFIG_PM_SLEEP would be better?
>>>
>>>
>>> the current implementation looks fine.
>>>
>>>> Also, can you move legacy suspend/resume
>>>
>>>
>>> Ok,
>>>
>>>> callbacks below to dev_pm_ops?
>>>>
>>>>>>>> + .suspend = ufshcd_pltfrm_suspend,
>>>>>>>> + .resume = ufshcd_pltfrm_resume,
>>>>>>>> +#endif
>>>>>>>> + .driver = {
>>>>>>>> + .name = "ufshcd",
>>>>>>>> + .owner = THIS_MODULE,
>>>>>>>> + .of_match_table = ufs_of_match,
>>>>>>>> + },
>>>>>>>> +};
>>>>
>>>>
>>>>
>>>> --
>>>> Regards,
>>>> Sujit Reddy Thumma
>>>>
>>>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>>>> member
>>>> of Code Aurora Forum, hosted by The Linux Foundation.
>>>
>>>
>>>
>>>
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
Regards,
Vinayak Holikatti
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
