Re: [PATCH V5 3/4] [SCSI] ufs: Add Platform glue driver for ufshcd

[Sujit Reddy Thumma <sthumma@codeaurora.org>]

On 1/9/2013 5:41 PM, vinayak holikatti wrote:
> On Mon, Jan 7, 2013 at 1:11 PM, Sujit Reddy Thumma
> <sthumma@codeaurora.org> 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.

>
>>>>>> + */
>>>>>> +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.
>
>
>