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

[Subhash Jadavani <subhashj@codeaurora.org>]

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

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.

>
>>
>>>>>>> + */
>>>>>>> +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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html