lkml.org 
[lkml]   [2016]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RESEND PATCH v5 2/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan
From
Date
Hi Adrian,

On 2016/3/4 20:12, Adrian Hunter wrote:

[...]

>>>
>>> I don't know that disabling clocks is necessarily the right thing to do if
>>> the resume fails. You might want to consider what happens if the system
>>> tries to use the device when it is in that state. It's been a long time
>>> since I did any ARM work but aren't you risking bus errors. Might just as
>>> well not bother and save a few lines of code.
>>>
>>
>> Firstly, I really just follow the orginal err handle.
>> It's risking bus errors if disabling ahb clk, but ahb clk is most very
>> likely to shared by many controllers. So regarding to the clk reference
>> count, the ahb_clk is not *really* disabled and just decrease the
>> reference count. How about keep these handles here?
>
> I would remove the error handling on the grounds that it does not do
> anything useful.

Take another ronud of thinking it, probably you are right. When failing
to resume from deelp sleep, we return err to dpm. dpm still roll-forward
to wakeup by calling resume callback one-by-one. It means that finally
when system in wakeup state with disabled ahb_clk, but we then going to
access mmc as normal, which is not acceptable. Especially for debuging
the failure, we have to enable ahb_clk manually.

Will remove the error handling.

Thanks.

>
>>
>>
>>>> + return ret;
>>>> }
>>>> #endif /* ! CONFIG_PM_SLEEP */
>>>>
>>>> @@ -183,6 +207,30 @@ static int sdhci_arasan_probe(struct platform_device
>>>> *pdev)
>>>> goto clk_disable_all;
>>>> }
>>>>
>>>> + sdhci_arasan->phy = ERR_PTR(-ENODEV);
>>>> + if (of_device_is_compatible(pdev->dev.of_node,
>>>> + "arasan,sdhci-5.1")) {
>>>> + sdhci_arasan->phy = devm_phy_get(&pdev->dev,
>>>> + "phy_arasan");
>>>> + if (IS_ERR(sdhci_arasan->phy)) {
>>>> + ret = PTR_ERR(sdhci_arasan->phy);
>>>> + dev_err(&pdev->dev, "No phy for arasan,sdhci-5.1.\n");
>>>> + goto clk_disable_all;
>>>> + }
>>>> +
>>>> + ret = phy_init(sdhci_arasan->phy);
>>>> + if (ret < 0) {
>>>> + dev_err(&pdev->dev, "phy_init err.\n");
>>>> + goto clk_disable_all;
>>>> + }
>>>> +
>>>> + ret = phy_power_on(sdhci_arasan->phy);
>>>> + if (ret < 0) {
>>>> + dev_err(&pdev->dev, "phy_power_on err.\n");
>>>> + goto err_phy_power;
>>>> + }
>>>> + }
>>>> +
>>>> ret = sdhci_add_host(host);
>>>> if (ret)
>>>> goto err_pltfm_free;
>>>> @@ -190,7 +238,12 @@ static int sdhci_arasan_probe(struct platform_device
>>>> *pdev)
>>>> return 0;
>>>>
>>>> err_pltfm_free:
>>>> + if (!IS_ERR(sdhci_arasan->phy))
>>>> + phy_power_off(sdhci_arasan->phy);
>>>> sdhci_pltfm_free(pdev);
>>>> +err_phy_power:
>>>> + if (!IS_ERR(sdhci_arasan->phy))
>>>
>>> Looks like you are using sdhci_arasan after it has been free'd by
>>> sdhci_pltfm_free().
>>>
>>> Also there seem to be cases where sdhci_arasan_probe() can exit after
>>> sdhci_pltfm_init() without calling sdhci_pltfm_free().
>>
>> Good catch, will fix it.
>>
>>>
>>>> + phy_exit(sdhci_arasan->phy);
>>>> clk_disable_all:
>>>> clk_disable_unprepare(clk_xin);
>>>> clk_dis_ahb:
>>>> @@ -205,6 +258,11 @@ static int sdhci_arasan_remove(struct
>>>> platform_device *pdev)
>>>> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>>> struct sdhci_arasan_data *sdhci_arasan = pltfm_host->priv;
>>>>
>>>> + if (!IS_ERR(sdhci_arasan->phy)) {
>>>> + phy_power_off(sdhci_arasan->phy);
>>>> + phy_exit(sdhci_arasan->phy);
>>>> + }
>>>> +
>>>
>>> When you re-base, take care to keep this chunk above
>>> sdhci_pltfm_unregister().
>>
>> sure.
>>
>> Thanks for reviewing it.
>>
>>>
>>>> clk_disable_unprepare(sdhci_arasan->clk_ahb);
>>>>
>>>> return sdhci_pltfm_unregister(pdev);
>>>>
>>>
>>>
>>>
>>>
>>
>>
>
>
>
>


--
Best Regards
Shawn Lin

\
 
 \ /
  Last update: 2016-03-05 03:41    [W:0.637 / U:0.148 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site